Motivation
We shouldn't crash on somewhat likely user error.
Modifications
Pass on writes after close(mode: .output) instead of crashing.
Result
User code is more robust to weird edge cases.
Motivation:
Currently the server pipeline handler ignores close. This generally works,
except in the rare case that the user calls close(mode: .output). In this
instance they have signalled that they'll never write again, and they're
likely expecting a final close shortly after.
However, it is possible that the pipeline handler has suspended reads
at the same time. On Linux this isn't an issue, because we'll still be told
about the eventual socket close. However, on Apple platforms we won't: we've
masked off the reads, and we can't listen to EVFILT_EXCEPT due to some
other issues. This means that on Apple platforms the server pipeline handler
can accidentally wedge the Channel open and prevent it from closing.
We should take this opportunity to have the server pipeline handler be smart
about close(mode: .output). What _should_ happen here is that the pipeline
handler should immediately refuse to deliver further requests on the Channel.
If one is in-flight, it can continue, but everything else should be dropped.
This is because the server cannot possibly respond to further requests.
Modifications:
- Add new states to the server pipeline handler
- Drop buffered requests and new data after close(mode: .output)
- Add tests
Result:
Server pipeline handler behaves way better.
Motivation:
The node.js HTTP parser library that we use has been unmaintained for some time. We should move to the maintained replacement, which is llhttp. This patch will update our dependency and bring us over to the new library, as well as make any changes we need.
Modifications:
This patch comes in 4 parts, each contained in a separate commit in the PR.
The first commit drops the existing http_parser code and updates some of the repo state for using llhttp.
The second commit rewrites the update script to bring in llhttp instead of http_parser.
The third runs the actual script. You can skip reviewing this except to sanity check the outcome.
The fourth commit updates the NIO code and the tests to get everything working.
In general the substance of the product modifications was minimal. The logic around keeping track of where we are in the buffer and how upgrades work has changed a bit, so that required some fiddling. I also had to add an error reporting path for the delegates to be able to throw specific errors that llhttp no longer checks for. Finally, I removed two tests that were a little overzealous and that llhttp does not police.
Result:
Back on the supported path.
Motivation:
As we've largely completed our move to split out our core abstractions,
we now have an opportunity to clean up our dependencies and imports. We
should arrange for everything to only import NIO if it actually needs
it, and to correctly express dependencies on NIOCore and NIOEmbedded
where they exist.
We aren't yet splitting out tests that only test functionality in
NIOCore, that will follow in a separate patch.
Modifications:
- Fixed up imports
- Made sure our protocols only require NIOCore.
Result:
Better expression of dependencies.
Co-authored-by: George Barnett <gbarnett@apple.com>
Motivation:
I'm sick of typing `.init(major: 1, minor: 1)`.
Modifications:
- Added static vars for common HTTP versions.
Result:
Maybe I'll never type `.init(major: 1, minor: 1)` ever again.
Motivation:
Without niling out it may linger around once the last test has
completed, and keep some messages in memory which will not be inspected
anymore
Modifications:
nil out the writeRecorder same as all other fields are currently nilled
out
Result:
In case XCTest keeps the class around, not keeping around the messages
stored by the writeRecorder
Motivation:
The HTTPServerPipelineHandler is removed from pipelines in many cases,
but the most common case is over upgrade. While the handler is
removable, it doesn't make any effort to ensure that it leaves the
pipeline in a sensible state, which is pretty awkward.
In particular, there are 3 things the pipeline handler may be holding
on to that can lead to damage. The first is pipelined requests: if there
are any, they should be delivered, as the user may be deliberately
allowing pipelining.
The second thing is read() calls. The HTTPServerPipelineHandler exerts
backpressure on clients that aggressively pipeline by refusing to read
from the socket. If that happens, and then the handler is removed from
the channel, it will "forget" to restart reading from the socket on the
way out. That leaves the channel quietly in a state where no reads will
occur ever again, which is pretty uncool.
The third thing is quiescing. The HTTPServerPipelineHandler catches
quiescing events and allows them to deliver a response before closing a
connection. If that has happened when the pipeline handler is removed,
it should fall back to the behaviour as though it were not there.
Modifications:
- Added a handlerRemoved implementation to play event state that should
be replayed.
- Added a channelInactive implementation to drop data.
Result:
More graceful handler removal.
Motivation:
`ctx` was always an abbreviation was 'context` and in Swift we don't
really use abbreviations, so let's fix it.
Modifications:
- rename all instances of `ctx` to `context`
Result:
- fixes#483
Motivation:
- `ChannelPipeline.add(name:handler:...)` had a strange order of arguments
- `remove(handler:)` and `remove(ctx:)` both remove `ChannelHandler`s
but they read like they remove different things
So let's just fix the argument order and name them `addHandler` and
`removeHandler` making clear what they do.
Modifications:
- rename all `ChannelPipeline.add(name:handler:...)`s to `ChannelPipeline.addHandler(_:name:...)`
- rename all `ChannelPipeline.remove(...)`s to `ChannelPipeline.removeHandler(...)`
Result:
more readable and consistent code
Motivation:
@pushkarnk hit an interesting edge case that we previously mishandled
and crashed. All these conditions need to be true
1. Whilst have an ongoing request (ie. outstanding response), ...
2. ... a `HTTPParserError` arrives (which we correctly queue)
3. Later the outstanding response is completed and then ...
4. ... the `HTTPServerProtocolErrorHandler` sends a .badRequest response
We previously crashed. The reason we crashed is that the
`HTTPServerPipelineHandler` obviously tracks state and then asserts that
no response is sent for a wrong request. It does have an affordance to
allow a .badRequest response for a request it couldn't parse. However
this state tracking wasn't done if the error itself was enqueued for
later delivery.
Thanks very much @pushkarnk for the report!
Modifications:
instead of delivering the error directly use the `deliverOneError`
function which transitions the state correctly.
Result:
fewer crashes & hopefully happy Pushkar
Motivation:
EmbeddedChannel just treated all `write`s as flushed. That's not a good
idea as all real channels obviously only actually write the bytes when
flushed. This changes that behaviour for `EmbeddedChannel`.
Modifications:
- make `write` only write to a buffer
- make `flush` then actually pretend to write the bytes
Result:
- EmbeddedChannel behaves more realisticly
Motivation:
In certain cases it's useful to quiesce a channel instead of just
closing them immediately for example when receiving a signal.
This lays the groundwork by introducing the
`ChannelShouldQuiesceUserEvent` user event that when received can be
interpreted by a ChannelHandler in a protocol & application specific
way. Some protocols support tear down and that would be a good place to
initiate the tear down.
Modifications:
- introduce `ChannelShouldQuiesceUserEvent`
- handle `ChannelShouldQuiesceUserEvent` in the `AcceptHandler` with
closing the server socket
- handle `ChannelShouldQuiesceUserEvent` in the
`HTTPServerPipelineHandler` by only handling a already in-flight
request and then no longer accepting input
- added `CircularBuffer.removeAll` (& tests)
- added tests for `nextPowerOf2()`
Result:
- handlers can now support quiescing
Motivation:
The pipelining handler made the assumption that `channelRead` is never
called recursively. That's mostly true but there is at least one
situation where that's not true:
- pipelining handler seen a response .end and delivers a .head (which is
done in `channelRead`)
- a handler further down stream writes and flushes some response data
- the flushes fail which leads to us draining the receive buffer
- if the receive buffer contained more requests, the pipelining
handler's `channelRead` is called again (recursively)
The net result of that was that the new request parts from the receive
buffer would now jump the queue and go through the channel pipeline
next, before other already buffered messages.
Modifications:
made the pipelining handler buffer if a `channelRead` comes in from the
pipeline and there is already at least one message buffered.
Result:
the ordering of the incoming messages should now be respected which is
very important...
Motivation:
HTTP pipelining can be tricky to handle properly on the server side.
In particular, it's very easy to write out of order or inconsistently
mutate state. Users often need help to handle this appropriately.
Modifications:
Added a HTTPServerPipelineHandler that only lets one request through
at a time.
Result:
Better servers that are more able to handle HTTP pipelining
Cory Benfield