Motivation
When we receive a HEAD response, it's possible that the response
contains a content-length. llhttp has a bug
(https://github.com/nodejs/llhttp/issues/202) that prevents it from
properly managing that issue, which causes us to incorrectly parse
responses.
Modifications
Forcibly set llhttp's content-length value to 0.
Result
Correctly handle HTTP framing around llhttp's issues.
Motivation:
We should better tolerate LLHTTP status codes we don't yet know about.
Modifications:
- Added support for the status codes that currently exist
- Add a fallback to the RAW case for the future.
Result:
Better management of LLHTTP status codes
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:
- Currently http 1xx response heads are handled incorrectly
Modifications:
- Add an `InformationalResponseStrategy` that allows the user to specify whether http 1xx responses shall be forwarded or dropped.
- Implement the necessary cases in `HTTPDecoder` and `BetterHTTPParser`
Co-authored-by: Cory Benfield <lukasa@apple.com>
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:
Non-implementation-only imports in Swift have an annoying tendency to
put C header files into all downstream build paths. The result is that
leaf packages that include any C header files run a very high risk of
colliding with declarations in the inner header files. In this case,
we've had users bump into issues with the HTTP_STATUS_* enum cases
already.
Modifications:
- Make CNIOHTTPParser import implementationOnly where possible.
Result:
Fewer risks of colliding symbols.
Motivation:
http-parser shipped a patche for node.js CVE-2019-15605, which allowed
HTTP request smuggling. This affected SwiftNIO as well, and so we need
to immediately ship an update to help protect affected users.
A CVE for SwiftNIO will follow, but as this patch is in the wild and
SwiftNIO is known to be affected we should not delay shipping this fix.
Modifications:
- Update http-parser.
- Add regression tests to validate this behaviour.
Result:
Close request smugging vector.
There is no `ChannelPipeline.addHTTPServerHandlers` but there is `ChannelPipeline.configureHTTPServerPipeline`.
Comment for `HTTPResponseDecoder` mentioning `ChannelPipeline.addHTTPClientHandlers` is correct.
Motivation:
Despite the fact that we already stopped delivering unsolicited
responses for the HTTP response decoder, there was a window where we
would deliver a second .head for a response that come without a request.
Modifications:
Don't deliver a second .head even if send together with a first, legit
response.
Result:
Being a HTTP client using NIO is now easier.
Motivation:
Right now NIO clients will crash in the (admittedly unlikely) event that
a HTTP server sends an unsolicited extra response on a connection. This
seems pretty unnecessary, and fairly graceless.
Modifications:
- Replace a forcible pop with an optional one.
- Added NIOHTTPDecoderError type.
Result:
Removed a crash that we definitely don't want.
Motivation:
Previously we thought that if we have some bytes left that belong to an
upgraded protocol, we should deliver those as an error. This is
implemented on `master` but not in any released NIO version.
However, no other handler sends errors on unclean shutdown, so it feels
wrong to do it in this one very specific case (EOF on inflight upgrade
with data for the upgraded protocol)
Modifications:
Remove the error again.
Result:
Less behaviour change to the last released NIO version.
Motivation:
Do as we documented and invoke the leftovers strategy only when the
handler is removed.
Modifications:
Don't invoke leftovers on EOF (which isn't something the user can
control).
Result:
- Fewer crashes
- should fix https://github.com/IBM-Swift/Kitura-WebSocket-NIO/issues/35
Motivation:
With HTTP/1.0 and EOF framing it's possible to have a HTTP response that
doesn't have any headers. HTTPDecoder unfortunately fell flat on its
face and crashed the program if that happened.
Modifications:
- don't expect headers on responses
- add tests for that
- rename the state machine cases to reflect a request and response
parsing
Result:
fewer crashes.
Motivation:
The HTTPDecoder is a complex object that has very careful state management goals. One source of this
complexity is that it is fed a stream of bytes with arbitrary chunk sizes, but needs to produce a
collection of objects that are contiguous in memory. For example, each header field name and value
must be turned into a String, which requires a contiguous sequence of bytes to do.
As a result, it is quite common to have a situation where the HTTPDecoder has only *part* of an
object that must be emitted atomically. In this situation, the HTTPDecoder would like to instruct
its ByteToMessageHandler to keep hold of the bytes that form the beginning of that object. To avoid
asking http_parser to parse those bytes twice, the HTTPDecoder uses a value called httpParserOffset
to keep track.
As an example, consider what would happen if the "Connection: keep-alive\r\n" header field was delivered
in two chunks: first "Connection: keep-al", and then "ive\r\n". The header field name can be emitted in
its entirety, but the partial field value must be preserved. To achieve this, the HTTPDecoder will store
an offset internally to keep track of which bytes have been parsed. In this case, the offset will be set
to 7: the number of bytes in "keep-al". It will then tell the rest of the code that only 12 bytes of the
original 19 byte message were consumed, causing the ByteToMessageHandler to preserve those 7 bytes.
However, when the next chunk is received, the ByteToMessageHandler will *replay* those bytes to
HTTPDecoder. To avoid parsing them a second time, HTTPDecoder keeps track of how many bytes it is
expecting to see replayed. This is the value in httpParserOffset.
Due to a logic error in the HTTPDecoder, the httpParserOffset field was never returned to zero.
This field would be modified whenever a partial field was received, but would never be returned
to zero when a complete message was parsed. This would cause the HTTPDecoder to unnecessarily keep
hold of extra bytes in the ByteToMessageHandler even when they were no longer needed. In some cases
the number could get smaller, such as when a new partial field was received, but it could never drop
to zero even when a complete HTTP message was receivedincremented.
Happily, due to the rest of the HTTPDecoder logic this never produced an invalid message: while
ByteToMessageHandler was repeatedly producing extra bytes, it never actually passed them to http_parser
again, or caused any other issue. The only situation in which a problem would occur is if the HTTPDecoder
had a RemoveAfterUpgradeStrategy other than .dropBytes. In that circumstance, decodeLast would not
consume any extra bytes, but those bytes would have remained in the buffer passed to decodeLast, which
would then incorrectly *forward them on*. This is the only circumstance in which this error manifested,
and in most applications it led to surprising and irregular crashes on connection teardown. In all
other applications the only effect was unnecessarily preserving a few tens of extra bytes on
some connections, until receiving EOF caused us to drop all that memory anyway.
Modifications:
- Return httpParserOffset to 0 when a full message has been delivered.
Result:
Fewer weird crashes.
Motivation:
Spotted a couple of issues with the documentation and fixed them.
Modifications:
- explicitly marked all `public class`es as `public final class` to get
consistency in the Jazzy documentation.
- removed `public` extension methods of the `internal struct
PriorityQueue` which Jazzy showed in the docs.
- improved the largely missing `EmbeddedChannel` and `EmbeddedEventLoop`
documentation.
- clear up lies in the B2MD docs
- add some other missing docs
Result:
better docs makes happier users
Motivation:
Before native UTF-8, the rawURI was a performance optimisation but
that's no longer true. Before this patch, we still had the rawURIs but
weren't using them at all.
Modifications:
remove any mention of rawURI
Result:
less clutter
Motivation:
The HTTP/1 headers were quite complicated, CoW-boxed and exposed their
guts (being a ByteBuffer). In Swift 5 this is no longer necessary
because of native UTF-8 Strings.
Modifications:
- make headers simply `[(String, String)]`
- remove `HTTPHeaderIndex`, `HTTPListHeaderIterator`, etc
Result:
- simpler and more performant code
Motivation:
The old HTTP parser tried to convert `http_parser` to a one-shot parser
by pausing it constantly. That was done to be resilient against
re-entrancy. But now, we have the new `ByteToMessageDecoder` which
protects against that so HTTP decoder can just become a real
`ByteToMessageDecoder`. It also serves as a great testbed for
`ByteToMessageDecoder`.
Modifications:
- make the HTTP decoder a `ByteToMessageDecoder`
- refactor the implementation
Result:
cleaner code, more use of `ByteToMessageDecoder`
Motivation:
Previously, ByteBuffers get* methods would get bytes from anywhere, even
if the bytes were not readable. That can lead to security issues in code
that doesn't expect this. NIO shouldn't contain such unsafe methods
without the word 'unsafe'.
Modifications:
Made all `get*` methods respect reader/writerIndex.
Result:
safer user code
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:
Some of the ChannelPipeline removal methods did not go through the
RemovableChannelHandler API.
Modifications:
make all removal APIs go through the RemovableChannelHandler API
Result:
fewer bugs
Motivation:
ByteBuffer methods like `set(string:)` never felt very Swift-like and
also didn't look the same as their counterparts like `getString(...)`.
Modifications:
- rename all `ByteBuffer.set/write(<type>:,...)` methods to
`ByteBuffer.set/write<Type>(...)`
- polyfill the old spellings in `_NIO1APIShims`
Result:
code more Swift-like
Motivation:
If ChannelHandler removal worked correctly, it was often either by
accident or by intricate knowledge about the implementation of the
ChannelHandler that is to be removed. Especially when it comes to
re-entrancy it mostly didn't work correctly.
Modifications:
- introduce a `RemovableChannelHandler` API
- raise allocation limit per HTTP connection by 1
(https://bugs.swift.org/browse/SR-9905)
Result:
Make things work by contruction rather than accident
Motivation:
initialRingCapacity is not a great name, initialCapacity is much better
Modifications:
rename CircularBuffer(initialRingCapacity:) to initialCapacity
Result:
more consistent code
Motivation:
- `Int` is the currency type for integral things and we already sort of
changed the the type for ports but not everywhere.
- generic parameters that aren't just 'any type' shouldn't be just `T`
or `S` or something
Modifications:
- make more use of `Int`
- rename generic parameters to something more descriptive like `Bytes`
Result:
easier to read/write NIO code
Motivation:
Explain here the context, and why you're making that change.
What is the problem you're trying to solve.
Modifications:
Describe the modifications you've done.
Result:
After your change, what will change.
Motivation:
Previously we used 0xdeadbeef for all dead pointers but that doesn't fit
into an `Int` on 32-bit platforms.
Modifications:
change the dead pointer value to 0xdeadbee which is good because even
after offsetting the pointer by a couple of bytes, we'll still have
'dead' in the registers which should give us a hint.
Result:
- unified dead pointer values on 32 and 64 bit
Motivation:
It's poor style to assume a certain piece of memory is bound to a
certain type across a whole program. That's because any piece of code
run in between might have rebound the memory and also it's hard to prove
that the binding assumption is actually correct. Therefore we should
restrain from using them.
Modifications:
remove more assumptions that memory is bound to a certain type.
Result:
better code
Motivation:
I came across a few typos and then went through the entire code base
fixing similar errors when I noticed a pattern.
Modifications:
No changes to the code, just to comments.
Result:
No changes.
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:
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:
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).
* Make NIOHTTP1 compile on 32-bit platforms
Motivation:
When building a datacenter out of Apple Watches w/ shattered
glasses, you'll need the ability to build on 32 bit platforms.
This fixes building NIOHTTP1 on such platforms.
Modifications:
This was resetting the parser context pointer using a very
long deadly beef. Had to shorted this for 32bit cows.
Result:
NIOHTTP1 compiles on 32 bit ARM platforms.
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:
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:
We stored a closure in the pending array which is not really needed. We can just store the NIOAny directly without creating a closure first.
Modifications:
Replace closure with NIOAny
Result:
Less creation of closures.
Motivation:
There are quite a few `switch`es that cover just two cases. Explicitly state this via an `if case` + `else` structure.
Modifications:
Changes something like:
switch a {
case .b:
return c
default:
return d
}
to
if case .b = a {
return c
} else {
return d
}
Result:
This should prevent misinterpretation and thus potential bugs in the future.
Motivation:
In order to create a `HTTPResponseStatus` value from an status code code
currently one would have to make a custom HTTPResponseStatus. The
problem with that is that:
`HTTPResponseStatus.custom(code: 200, reasonPhrase: "") != HTTPResponseStatus.ok`
The logic of detecting whether a status code matches one of the known
cases already exists but only in an internal factory method.
Modifications:
- removed internal static `from` method from `HTTPResponseStatus`
- added public init with the functionality previously in `from`
- replace the single use of the `from` method with the new `init`
Result:
When initializing a `HTTPResponseStatus` value with a value of e.g.
`200` the init method will result in a `HTTPResponseStatus.ok`.
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.
David Nadoba