Motivation:
Our copy of http_parser has become almost a year old now, and there are
both some bugfixes that improve correctness and some performance
improvements in http_parser that we should pick up.
Modifications:
- Updated http_parser
- Added tests to confirm we don't suffer from
https://github.com/nodejs/http-parser/pull/432
- Added tests that we didn't get broken by SOURCE verb addition.
Result:
Shiny new http_parser!
Motivation:
Swift has a rather unhelpful warning
warning: treating a forced downcast to 'EmbeddedEventLoop' as optional will never produce 'nil'
when assigning that to a variable of type `EmbeddedEventLoop!`. The
warning is accurate but obviously we know that can never be `nil`. The
way to silence this warning is to put parenthesis around the expression.
Modifications:
added parenthesis around force casts that are assigned to IUOs
Result:
fewer warnings with 4.2
Motivation:
testUpgradeWithUpgradePayloadInlineWithRequestWorks used the
EmbeddedEventLoop to allocate some futures but setUpTestWithAutoremoval
spawns a MultiThreadedEventLoopGroup. Hence, threading was broken in the
test and this PR fixes it.
Modifications:
use a MultiThreadedEventLoopGroup instead of an EmbeddedEventLoop
Result:
tests pass in TSan again
Motivation:
We should be consistent with naming and also choose descriptive names.
Modifications:
- Deprecate old init method that uses numThreads
- Add new init with numberOfThreads param name
- Everywhere use the new init
Result:
More consistent and descriptive naming. Fixes https://github.com/apple/swift-nio/issues/432.
Motivation:
testUpgradeWithUpgradePayloadInlineWithRequestWorks has a race condition
where two bytes could be read in one go whereas the expectation was that
they're sent separately.
Modifications:
fix the above race
Result:
- more stable tests
- fixes#434
Motivation:
d53ee6dafc introduced a new constructor to HTTPRequestDecoder which allows to change the behaviour of handling left over bytes when an upgrade was detected and the decoder was removed. This was done as an internal init block as we wanted to to do as part of a patch release.
Modifications:
Change internal to public init
Result:
More flexible configuration possible.
Motivation:
398b950eee introduced a change which forwarded bytes that were buffered in HTTPDecoder after it was removed when an upgrade was detected. This may be risky if we dont know that the pipeline can handle it.
Modifications:
- Add new initializer that allow us to configure if we should forward bytes or not.
- Automatically use this if we know that we added an upgrader
- Add unit tests.
Result:
Less risky implementation.
Motivation:
We need to ensure we correctly guard against re-entrancy for all cases. Also we did not correctly ensure we forward pending data after removal which could lead to missing data after upgrades.
Modifications:
- pause the parser when we need to callout to the pipeline and so ensure we never fire events throught the pipeline while in callbacks of http_parser
- correctly forward any pending bytes if an upgrade happened when the decoder is removed
- Ensure HTTPUpgradeHandler only remove decoder after the full request is received
- Add testcase provided by @weissi
Result:
Correctly handle upgrades and pending data. Fixes https://github.com/apple/swift-nio/issues/422.
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:
To detect if keepalive is used we need to search the headers (if HTTP/1.1 is used). This may be expensive depending how many headers are present. http_parser itself detects already if keep alive should be used and so we can re-use this as long as the user did not modify the headers.
Modifications:
Reuse keepalive parsed by http_parser as long as the headers were not modified.
Result:
Less overhead as long as the headers are not modified.
Motivation:
When discardReadBytes() was called and we still did not pass the Head to the user it was quite possible that the headers / URI could be corrupted as the stored readerIndex did not matchup anymore.
Modifications:
- Override most of the functionality of ByteToMessageDecoder in HTTPDecoder to better work with the provided state machine of http_parser
- Remove allocations by removing the pendingInOut Array as its not needed anymore.
- Add guards against re-entrance calls
- Add unit test that shows that everything works as expected now.
Result:
Fixed bug and reduced allocations (8% - 10% perf win).
Motivation:
HTTP{Request,Response}Head were too large even with a less than 3 word
ByteBuffer, this shrinks them box CoW boxing them. Should also reduce
ARC traffic when passing around a bunch.
Modifications:
CoW box HTTP{Request,Response}Head
Result:
fewer existential containers created, more performance
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:
We had a bug which is happens in the combination of these states:
- we held a request in the pipelining handler (because we're procesing a
previous one)
- a http handler error happened whilst a response's `.head` had already
been sent (but not yet the `.end`)
- the HTTPServerProtocolErrors handler is in use
That would lead to this situation:
- the error isn't held by the pipelining handler
- the error handler then just sends a full response (`.head` and `.end`)
but the actual http server already send a `.head`. So all in all, we
sent `.head`, `.head`, `.end` which is illegal
- the pipelining handler didn't notice this and beause it saw an `.end`
it would send through the next requst
- now the http server handler is in the situation that it gets `.head`,
`.head` too (which is illegal)
Modifications:
- hold HTTP errors in the pipelining handler too
Result:
- more correctness
Motivation:
Its very likely that the response will be HTTP/1.1 or HTTP/1.0 so we can optimize for it by minimizing the buffer.write(...) calls.
Beside this we should also change the pattern of *.write(into: inout ByteBuffer) to extension on ByteBuffer itself.
Modificiations:
- Optimize for HTTP/1.0 and 1.1
- Use extensions on ByteBuffer
Result:
Faster and more clean code.
Motivation:
HTTP message framing has a number of edge cases that NIO currently does
not tolerate. We should decide what our position is on each of these edge
cases and handle it appropriately.
Modifications:
Provide an extensive test suite that codifies our expected handling of
these edge cases. Fix divergences from this behaviour.
Result:
Better tolerance for the weird corners of HTTP.
Motivation:
We are currently parsing each header eagly which means we need to convert from bytes to String frequently. The reality is that most of the times the user is not really interested in all the headers and so it is kind of wasteful to do so.
Modification:
Rewrite our internal storage of HTTPHeaders to use a ByteBuffer as internal storage and so only parse headers on demand.
Result:
Less overhead for parsing headers.
Motivation:
HTTPHeaders had an unusual API for Swift: `getCanonicalForm(String)` which is
better suited as a subscript `[canonicalForm: String]`.
Also the implementation was needlessly inefficient.
Modifications:
- renamed HTTPHeaders.getCanonicalForm to HTTPHeaders[canonicalForm]
- improved efficiency by replacing anti-pattern
Array.map { ... }.reduce(+, []) by flatMap
Result:
more Swift in both meanings
Motivation:
Currently the HTTP decoders can throw errors, but they will be ignored
and lead to a simple EOF. That's not ideal: in most cases we should make
a best-effort attempt to send a 4XX error code before we shut the client
down.
Modifications:
Provided a new ChannelHandler that generates 400 errors when the HTTP
decoder fails.
Added a flag to automatically add that handler to the channel pipeline.
Added the handler to the HTTP sample server.
Enabled integration test 12.
Result:
Easier error handling for HTTP servers.
Motivation:
Our HTTP code handles only HTTP/1.X. There is no reason to support
HTTP/0.9, and we cannot safely handle a major protocol higher than 1 in
this code, so we should simply treat requests/responses claiming to be
of those protocols as errors.
Modifications:
HTTPDecoder now checks the major version is equal to 1 before it
continues with parsing. If it hits an error, that error will be propagated
out to the user.
Result:
Better resilience against bad HTTP messages.
Motivation:
Fix inconsistency of <Void> and <()> occurances in the codebase.
Modifications:
Changed EventLoop{Future,Promise}<()> occurances to EventLoop{Future,Promise}<Void>
Result:
Consistent code style for EventLoopPromise and EventLoopFuture.
Motivation:
Changes the Boolean `fulfilled` to `isFulfilled` to make it more clear it is in fact a Boolean.
Also changes a `func isRecoverable()` to `var isRecoverable`.
Both of these were non-public, so this is no breaking change.
Modifications:
Renamed one Boolean and changed one function to a variable.
Result:
No use of unncessary (and potentially misinforming) functions, as well as adhering to the Swift boolean naming guidelines.
Motivation:
The current HTTP server helpers are not modular, meaning that each
time we add a new first-party HTTP handler we need to add a brand new
method. Additionally, they do not currently compose, so if you want
assistance with HTTP pipelining *and* to support HTTP upgrade you are
out of luck.
Modifications:
Deprecate the two existing server helpers.
Add a new server helper that can be extended to support all of the
possible features that users may want in their HTTP pipelines.
Result:
Users will have a single place to go that can be used to configure
their HTTP server pipeline, and that understands how the different
handlers fit together in a complete pipeline.
Motivation:
The HTTP pipeline may contain other HTTP protocol specific handlers,
beyond just the ones that the upgrade handler knows explicitly. It should
be possible to get the upgrade handler to remove those at the right time.
Modifications:
Add extra support for removing handlers.
Result:
It'll be possible for the HTTP upgrade logic to remove things like the
pipelining handler at upgrade time.
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
Motivation:
The previous delayed upgrade test didn't really validate what it
claimed to be doing: it validated properties of the I/O, which made
it enormously flaky.
Modifications:
Change the tests to check for the presence/absence of the server
upgrade handler, which is a mark of the actual upgrade process.
Result:
Less flaky tests
Motivation:
The upgrade test would race with itself and occasionally fail.
Modifications:
Give enough time for all the work to settle before closing the
client channel.
Result:
No more flaky tests.
Motivation:
Sometimes we deviated from the style the Swift stdlib sets out for no
reason.
Modifications:
Fixed some stylistic deviations.
Result:
Looks more Swift-like.
Motivation:
Currently the HTTPUpgradeHandler does not allow for the protocol
upgraders to return EventLoopFuture objects. This leads to an
awkward scenario: it's essentially required that an upgrader be able
to synchronously change the pipeline to its satisfaction and prepare for
more bytes.
This is not really possible: ChannelPipeline.add(handler:) returns a
Future, which means that it is possible for this operation, at least
in principle, to not execute synchronously. This reality puts the
HTTPUpgradeHandler at odds with the reality of channel pipelines, and
cannot stand.
Modifications:
Give the HTTPProtocolUpgrader protocol to have upgrade() return an
EventLoopFuture<Void>, and update the HTTPServerUpgradeHandler to
appropriately handle the returned future by buffering until the future
completes.
Result:
It is no longer necessary to synchronously change the channel
pipeline during protocol upgrade.
Motivation:
Right now the test gen script will rewrite every file. Not ideal!
Modifications:
Added the license header, and made the follow-on comment match that
style.
Result:
Test files will look the same when running this script as they do
now.
Motivation:
The HTTP upgrade handler should not require that we case-match with
the mandatory headers.
Modifications:
Treat all headers as lowercase.
Result:
Upgrade is easier.
Motivation:
guard !self.open else { ... } is a double negation and can be confusing,
guard self.isOpen else { ... } is much better
Modifications:
replaced all `var closed: Bool` with `var isOpen: Bool`
Result:
we're more positive
Motivation:
We have multiple reasons why flush promises weren't great, for example:
- writeAndFlush sounds like it's a write + flush optimisation but in
reality it was more expensive (2 extra promises)
- the semantics were never quite clear
- lots of implementation complexity, especially for the datagram channel
Modifications:
- removed the flush promises in the API
- this deliberately doesn't do the PendingWritesManager simplifications
that this unlocks
Result:
flush doesn't have a promise anymore.
Motivation:
There were a couple of places in the Channel implementations that just
weren't thread-safe at all, namely:
- localAddress
- remoetAddress
- parent
those are now fixed. I also re-ordered the code so it should be easier
to maintain in the future (the `// MARK` markers were totally
incorrect).
Modifications:
- made Channel.{local,remote}Address return a future
- made Channel.parent a `let`
- unified more of `SocketChannel` and `DatagramSocketChannel`
- fixed the `// MARK`s by reordering code
- annotated methods/properties that are in the Channel API and need to
be thread-safe
Result:
slightly more thread-safety :)
Motivation:
Foundation is problematic for a few reasons:
- its implementation is different on Linux and on macOS which means our
macOS tests might be inaccurate
- on macOS it uses ObjC Foundation which means the autorelease pool
might get populated
- it links the world on Linux which means we can't do static
binaries at all
Modifications:
removed the last bits of Foundation dependency
Result:
no Foundation dependency
Motivation:
For HTTP parsing, we used Foundation to trim whitespace which is not
needed.
Modifications:
Implemented whitespace trimming in Swift.
Result:
less Foundation
to not return a value
Motivation:
We recently had a bug where we had `EventLoopFuture<EventLoopFuture<()>>` which didn't make any sense. The compiler couldn't catch that problem because we just ignored a closure's argument like this:
future.then { _ in
...
}
which is dangerous. For closures that take an empty tuple, the `_ in`
isn't actually required and the others should state the type they want
to ignore.
And most whenComplete calls can be better (and often shorter) expressed
by other combinators.
Modifications:
remove pretty much all closures which just blanket ignore their
parameter.
Result:
- no closures which just ignore their parameter without at least stating
its type.
- rewrote all whenCompletes that actually used the value
Motivation:
Previously `FileRegion` was a special snow flake, a bit like
`ByteBuffer` but also totally different. That caused surprise and made
`PendingWritesManager` even harder. It was also used to manage the
lifetime of a file descriptor but only sort of.
Modifications:
We now have a `FileHandle` which is a one-to-one mapping with a file
descriptor and its lifetime must be managed appropriately.
Result:
hopefully less bugs and fd leaks.
Motivation:
Lots of our most important operations had redundant labels like
func write(data: NIOAny)
the `data: ` label doesn't add anything meaningful and therefore it
should be removed.
Modifications:
removed lots of redundant labels
Result:
less redundant labels
Motivation:
We can allocate the buffer on the fly if needed, no need to add the constructor argument.
Modifications:
Just allocate the buffer in the methods itself that need it.
Result:
Easier to construct encoder and cleaner code.
Motivation:
Often people want to transfer large files over http which should be done using sendfile(...) for best performance.
Modifications:
- Add support for using FileRegion when sending body parts.
- Add tests
Result:
Be able to serve files without loading these into memory
* HTTP/1.1 Chunked Encoding
* fix MarkedCircularBuffer: if not marked do not try to move mark when removing first
* fix HTTPRequestDecoder: dispatch callouts after parsing is complete
* fix HTTPTest: feed pipeline the inbound data correctly
* Make HTTPHeaders iterate with originalcase'd names
Interface change as a consequence of this: now iterating headers
produces (name, value) tuples rather than (name, [values]).
* Add linux test wrapper
* Add case insensitive lookup test
* Update generate test headers
* Make HTTPHeadersIterator private
* Massage HTTPHeaders
Simple had it's own version that was the same aside from a couple of
minor tweaks.
* Default initializer for HTTPHeaders
* Rename HTTPRequest -> HTTPRequestPart
* HTTPResponse -> HTTPResponsePart
* Add HTTPVersion(major:,minor:)
* Allow access to HTTPVersion.major/minor
* Comments on HTTP response codes
* Fix tests (needed rename applied)
Cory Benfield