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