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:
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
* Reorder ‘channel active’ calls to the same order as `_ChannelInboundHandler` and their likely chronological order.
Motivation:
When first viewing the example classes, coming to the ‘read’ method first, leaves the subject unclear as to what the method is ‘reading’.
It is preferable to view something being sent first, and then to view the reading of the response.
It also matches the call order against the protocol making it a little easier for those unfamiliar with the protocol to see which methods have been implemented.
Modifications:
Moved channel active calls to be top of the class.
Despite the diff there are no actual code modifications.
UDP Client changed to indent using spaces to match rest of project. Incidental change.
Result:
The examples are slighter clearer to read, particularly for newcomers to swift-no as the calls are in a logical chronological order.
* Fix an error in the upgrader and its tests which checked the connection header incorrectly.
Motivation:
To correct the logic in the upgrader. Previously it checked if the upgraders required headers include the connection header. Now it checks that the connection header value was present as a separate header in the incoming request.
To prepare the class naming for the presence of a client upgrader.
Modifications:
Slight change in server upgrader handler logic with accompanying tests.
Renamed upgrader and test files to include the word ‘server’.
Ran the linux test script.
Result:
The server upgrader now checks for the presence of the connection header value as a header of its own.
* Rename WebSocketServer upgrader to make the naming clearer and make way for a client version.
Motivation:
To make the web socket upgrader naming clearer, particularly once we add a client version.
Modifications:
Rename WebSocketUpgrader too WebSocketServerUpgrader.
Result:
Improved clarity of naming on the web socket upgrader.
* Adds correct naming conventions to NIOWebSocketServerUpgrader. Sets back an incorrect fix (non-fix).
* Move deprecation for NIOWebSocketServerUpgrader typealias out of API shims file.
* Makes HTTPEncoder removable.
Adds a client upgrade handler with tests.
Adds the ability to use a client upgrader to the client setup.
Initial linux file update.
* Allow assertContains to be accessed publicly, so that it can be used in the client side tests.
* Update tests to remove server upgrader from client tests.
Change client tests to use Embedded channel.
Update HTTPUpgradeConfiguration class name to be NIOHTTPServerUpgradeConfiguration
Few other small stylistic changes.
* Removed awaiting upgrader state.
Removed future from handle upgrade call as is synchronous.
Removed protocol iterator from handle upgrade call as is not required by the client.
* Ensure that the client handler calls for the HTTPPipeline are backwards compatible.
Ensure that incoming promises to the handler write call are completed.
Neaten the upgrade header formation to remove looping.
Improve the correctness of the upgrade error throwing.
* Update scripts to match new unit tests.
* Change the documentation for HTTPServerPipeline to remove nil options which have now been removed.
* Restore an incorrectly added server pipeline comment change and make it to the client pipeline instead.
* Raise the allocation limits in the docker images to allow the tests to pass.
* Allow adding a sequence of headers to HTTPHeaders
Motivation:
When combining two collections of headers, adding each pair one-by-one
using add(name:value:) can cause multiple unnecessary reallocations of
the underlying array.
Modifications:
Provide two additional add methods to HTTPHeaders: one to add a sequence
of name/value pairs and another to add the headers from another
HTTPHeaders, both of which use `append(contentsOf:)` on the underlying
array to avoid additional reallocations.
Result:
Fewer reallocations when adding multiple headers.
* Fix incorrect documentation, make add<S: Sequence>(contentsOf:) @inlineable
* Reserve capacity and add sequentially instead of using append(contentsOf:)
* Add tests and missing @usableFromInline
Motivation:
When writing B2MDs, there are a couple of scenarios that always need to
be tested: firehose feeding, drip feeding, many messages, ...
It's tedious writing those tests over and over again for every B2MD.
Modifications:
- Add a simple B2MD verifier that users can use in unit tests.
- Add a new, public `NIOTestUtils` module which contains utilities
mostly useful for testing. Crucially however, it does not depend on
`XCTest` so it can be used it all targets.
Result:
Hopefully fewer bugs in B2MDs.
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:
To make the web socket upgrader naming clearer, particularly once we add a client version.
Modifications:
Rename WebSocketUpgrader too WebSocketServerUpgrader.
Result:
Improved clarity of naming on the web socket upgrader.
Motivation:
https://github.com/apple/swift-nio/issues/701
Modifications:
* Add `init(rawValue:)`
* Add `rawValue`
* Add tests in HTTPTypesTest.swift
Result:
Users no longer have to write their own conversion methods.
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:
EmbeddedChannel.finish/writeInbound/writeOutbound returned some mystery
bools. I always had to check the code to remember what they actually
meaned.
Whilst this is technically a breaking change, I couldn't find any users
of the return values on Github that are using the convergence releases.
Modifications:
Replace them by enums giving you all the information.
Result:
- fixes#916
- clearer APIs
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:
Use better protocol instead of Sequence since it doesn't do much. Inspired by #676
Modifications:
Conform HTTPHeaders to RandomAccessCollection
Result:
HTTPHeaders will have RandomAccessCollection functionalities.
Motivation:
We're not happy with HTTPResponseCompressor's API and it needs to
incubate a little more, hence moving to
[`swift-nio-extras`](https://github.com/apple/swift-nio-extras).
Modifications:
- removed HTTPResponseDecoder
- removed the zlib dependency
Result:
- no more HTTPResponseDecoder
Motivation:
readInbound/Outbound will soon throw errors if the type isn't right.
Modifications:
prepare tests for throwing readIn/Outbound
Result:
@ianpartridge's PR should pretty much merge after this.
Motivation:
While the HTTPServerUpgrader and WebSocketUpgrader allowed users to take
their time when reconfiguring the pipeline after they decided to upgrade,
users had to synchronously decide if they wanted to upgrade. This is a bit
inconvenient.
A particular limitation here is that some routes may want to upgrade
only if the upgrading user is authenticated. Checking authentication
credentials is almost always an I/O operation, and so cannot safely be done
on the event loop. Our original design made this impossible.
Modifications:
- Changed the shouldUpgrade callback to return a Future.
- Passed the shouldUpgrade callback the Channel that is upgrading, in
no small part so that it has an EventLoop it can create Futures
on.
- Rewrote the upgrader to handle this new state.
Result:
Users can delay decisions about when to upgrade.
Motivation:
There's just too much test code that has
(channel.eventLoop as! EmbeddedEventLoop).run()
Modifications:
have EmbeddedChannel expose its EmbeddedEventLoop
Result:
easier test 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:
- `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:
Collisions with Foundation and other standard libraries is painful.
Modifications:
- rename `FileHandle` to `NIOFileHandle`
- rename `Thread` to `NIOThread` (only exposed in docs)
Result:
fewer collisions
### Motivation:
A convenience/shorthand for developers.
### Modifications:
Add conformance to ExpressibleByDictionaryLiteral along with implementation.
### Result:
The following is would then be legal in Vapor:
```
return HTTPResponse(status: .ok, headers: ["Content-Type": "text/html"],
body: "<table border=1><tr><td><i>Hello</i>, plugin!")
```
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
Motiviation:
After adding `ELF.whenAllComplete` the concept of "fail fast" and "fail slow" for reducing an array of future results together was introduced.
This commit adds that concepts with the `andAll* methods that act as simple completion notifications.
Modifications:
Rename `EventLoopFuture.andAll(_:eventLoop:)` to `andAllSucceed(_🔛)` to denote its "fail fast" nature, and to match Swift API guidelines.
Add new `EventLoopFuture.andAllComplete(_🔛)` for a "fail slow" companion.
Shift implementation of `whenAllComplete(_🔛)` to be usable without unnecessary allocations in `andAllComplete`
Result:
EventLoopFuture now has two methods for "flattening" arrays of EventLoopFuture into a single notification ELF
Remove EmbeddedChannel restriction to IOData
Motivation:
You should be able to read any type from the outbound buffer.
Modifications:
Change Embedded channel outboundBuffer and pendingOutboundBuffer to store NIOAny instead of IOData and refactor test code accordingly.
Result:
- Users can test what went through the pipeline without serializing everything and using "if case let" syntax everywhere.
- fixes#551
Add deadlines as an alternative to timeouts
Motivation:
Address #603
Modifications:
- Add NIODeadline type
- Add func scheduleTask<T>(deadline: NIODeadline, _ task: @escaping () throws -> T) -> Scheduled<T>
- Reduce the use of DispatchTime in favor of Time
- Replace timeout calls with deadline calls where it makes sense
Result:
Tasks can be scheduled after an amount of time has passed, or at a certain time.
Motivation:
The name `mapIfError` implied that you can transform the error value but
really that's never what it did. Instead, it allowed you to recover
from a failure.
Modifications:
- rename `mapIfError` to `recover`
Result:
cleaner method names
Motivation:
EventLoopPromise.succeed/fail has extraneous labels that don't add
anything but noise.
Modifications:
remove extraneous labels
Result:
less noise in the code
Motivation:
ELF's API should be as close as possible to the new Result's API.
Therefore, we should rename `then` to `flatMap`
Modifications:
- renamed `then` to `flatMap`
- renamed `thenIfError` to `flatMapError`
- renamed ELF's generic parameter from `T` to `Value`
Result:
- more like Result
- fixes#688
Motivation:
When we write unit tests, we generally like them to actually test the
thing they claim to. This test never ran, and the assertions in place
to verify that it ran also never ran, so we didn't notice the test
not running.
Modifications:
- Moved the assertion that validates the test ran to a place where
it will definitely execute.
- Ensured the EmbeddedChannel gets marked as active, so that we
actually deliver the bytes from the HTTPDecoder.
Result:
This test will run, and actually pass.
Motivation:
Now that the stdlib has introduced the Result type, we can use it in the
implementation (and the whenComplete) function of EventLoopFuture
Modifications:
- replace EventLoopValue with Result
- make whenComplete provide the Result
Result:
use the new shiny stuff
Motivation:
The latest Swift 5.0s bring us new goodies (hence tools version) and
also more warnings, hence the fixes for them.
Modifications:
- set tools-version to 5.0
- fix warnings
- workaround https://bugs.swift.org/browse/SR-9514
Result:
happier developers
Motivation:
Swift naming guidelines mandate that factory methods start with `make`,
like `makeSomething`. We had a few that were `newSomething`.
Modifications:
make all factories start with make
Result:
more compliant to Swift naming rules
Motivation:
NIO2 development starts now.
Modifications:
Made NIO Swift 5-only for everything else see docs/public-api-changes-NIO1-to-NIO2.md
Result:
NIO2 development can start.
Motivation:
making a promise of a type is more grammatical than making a promise for
a type.
Modifications:
s/newPromise(for/newPromise(of/g
Result:
more grammar
Motivation:
When creating new promises I always find it very frustrating to type the
`: EventLoopPromise<Type>` type annotation but it's necessary for the
compiler to know type the promise will be fulfilled with.
Modifications:
allow an optional `for: SomeType.self` parameter for `newPromise` as
let p = eventLoop.newPromise(for: Int.self)
is much easier to type than
let p: EventLoopPromise<Int> = eventLoop.newPromise()
Result:
easier to write code
* CONNECT method has no request body
### Motivation:
[rfc7231](https://tools.ietf.org/html/rfc7231#section-4.3.6) said:
> A payload within a `CONNECT` request message has no defined semantics;
sending a payload body on a `CONNECT` request might cause some existing
implementations to reject the request.
### Modifications:
Change `HTTPMethod.CONNECT.hasRequestBody` to returns `.no`.
### Result:
`HTTPRequestEncoder` does not generate chunked body for `CONNECT` method.
* Change `HTTPMethod.CONNECT.hasRequestBody` to returns `.unlikely`
Motivation:
There was an outstanding change in http_parser.c that we did not yet
have in our copy.
Modifications:
run the update http parser script and add changes as well as a test case
designed to hit the change.
Result:
ship the latest and greatest
### Motivation:
make swift-nio compatible with Android, and pass all test in Android.
### Modifications:
* add missed `ifaddrs` implementation
* add missed thread related functions - `CNIOLinux_pthread_getname_np`, `CNIOLinux_pthread_setaffinity_np` and `CNIOLinux_pthread_getaffinity_np`
* fix types incositency between Linux and Android, e.g. `Epoll.swift` class
* make bytes counter explicitely 64bits to avoid overflow on 32bit, in `PendingWritesManager.swift` and `PendingDatagramWritesState.swift`
* fix issue with Unix domain socket, first byte need to be zero in Android
* several incosistency fixes between Linux and Android api.
### Result:
now swift-nio works on Android. All tests passed!
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:
HTTP/2 adds an additional item to a header key/value pair:
its indexability. By default, headers can be inserted into
the HPACK header table index; sometimes this will be denied
(common case is 'content-length' header, which is rarely going
to be the same, and will just cause the table to purge more
often than is really necessary). Additionally, a header may be
marked as not only non-indexable but also non-rewritable by
proxies. HTTPHeaders provides nowhere to store this, so the
HPACKHeaders implementation referenced in
apple/swift-nio-http2#10 was created to add in that capability.
Now, given that we really want the HTTP version to be an
implementation detail, we want to keep the HPACK details hidden,
and would thus be using HTTPHeaders in client APIs. NIOHTTP2
already has a HTTP1-to-HTTP2 channel handler that translates
between the two. Thus it behooves us to have the means to copy
the actual key/value pairs between the two types without making
a round-trip from UTF-8 bytes to UTF-16 Strings. These changes
allow NIOHPACK or NIOHTTP2 to implement that round-trip internally.
Modifications:
- HTTPHeader and HTTPHeaderIndex types are now public.
- HTTPHeaders.buffer and HTTPHeaders.headers properties are now
public.
- A new static method, HTTPHeaders.createHeaderBlock(buffer:headers:)
was created to serve as a public means to create a HTTPHeaders from
a ByteBuffer from outside NIOHTTP1. @Lukasa suggested this should
be a static method rather than an initializer.
Result:
Nothing in NIOHTTP1 changes in operation. All public types have
documentation comments noting that they are only public for very
specific reasons, and are not intended for general use. Once this
is committed, NIOHPACK & NIOHTTP2 will be able to implement fast
round-trips between HTTPHeaders and HPACKHeaders.
match all values in HTTPHeaders.isKeepAlive(...)
HTTPHeaders.isKeepAlive(...) does only match the first value.
### Motivation:
Keep-alive and Close may be something on comma separated arrays
### Modifications:
- Made an extension function to `ByteBuffer` that could separate the strings at low level, without Swift string API, so that it could split the values
- Another extension functions that compares the comma separated array with a given array to tell the caller which exists and which no
- Added unit tests for them
- Used them in the HTTPHeader
### Result:
- Now, if a request is sent using Keep-alive or Close where it was in an array, it will be handled
- fixes#410
Motivation:
Naked `try`s in tests are a problem because if they're hit you'll get a
bad error message and likely no file/line information. That has made
many flaky tests harder to debug than necessary.
Modifications:
- fix a lot of naked `try`s
- symlink `TestUtils` from `NIOTests` into `NIOHTTP1Tests`
- make a bunch of `wait()`s synchronous that were asynchronous by
accident
Result:
- better code
- easier to debug test failures
- setting better example
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
Cory Benfield