Add a DocC article on promise and event ordering in Channel implementations - #3693
Add a DocC article on promise and event ordering in Channel implementations#3693fabianfett wants to merge 2 commits into
Conversation
…ations Motivation: The order in which a `Channel` fulfills an operation's promise and fires the matching pipeline event is a guarantee handlers depend on, but it was only written down implicitly, spread across NIOPosix and ChannelNotificationTest. Anyone implementing ChannelCore had to reverse engineer it. Modifications: Add Sources/NIOCore/Docs.docc/channel-notification-order.md, documenting the state -> promise -> pipeline event -> closeFuture ordering, with per-operation walkthroughs for registration, activation, closing, half-closure and writability, plus sections on error reporting, re-entrancy, guards and finally an short implementation checklist. Link it from the NIOCore article index. Result: - `ChannelCore` implementers have a written reference for the notification order. - No functional change.
| its promises and fire its pipeline events. | ||
|
|
||
| Almost every operation on a ``Channel`` has two visible outcomes. The first one | ||
| is private to the caller: the ``EventLoopPromise`` that was passed to the |
There was a problem hiding this comment.
This framing isn't quite right. This object isn't private to the caller: it's passed up and down the channel pipeline.
| protocol. Its methods are public, but they exist for the use of the ``Channel`` | ||
| implementation itself and should only ever be called from the channel's | ||
| ``EventLoop``. If you are writing a ``ChannelHandler``, read this article as a | ||
| description of the guarantees you are given, and keep using |
There was a problem hiding this comment.
This is not just a description of the guarantees you're given, but the ones you are expected to continue to uphold if you, for example, interact with the promises or the events.
|
|
||
| The rest of this article works through each operation in turn. | ||
|
|
||
| ### Registration |
There was a problem hiding this comment.
We may want to leverage this section in future to talk about what these events mean, but for now this is totally fine.
| performs the `bind` and `listen` syscalls, and only in the success continuation | ||
| does it drive the activation, which in turn fulfills the user's bind promise. | ||
| The comment in that code is blunt about it: it is important to call the state | ||
| changing methods before notifying the original promise, for ordering reasons. |
There was a problem hiding this comment.
I'm not sure that this last note about the comment adds much.
| - A **registration** failure does both: it fires `errorCaught`, closes the | ||
| channel, and fails the register promise. A failing re-registration does the | ||
| same, minus the promise, because there is none. | ||
| - A failing **deregistration** during a close is fired as `errorCaught`, whereas |
There was a problem hiding this comment.
Might be worth calling out that it isn't really possible for deregistration to fail in a meaningful way.
| 4. **Fire ``Channel/closeFuture`` last of all**, and only when the channel is | ||
| being torn down. This happens after the pipeline has been dismantled, on a | ||
| later event loop tick. |
There was a problem hiding this comment.
Closing is a special case so it may be worth removing it from here and the single sentence below, and instead calling it out as special and covered in more detail below.
| promise?.fail(ChannelError.ioOnClosedChannel) | ||
| return | ||
| } | ||
| guard !self.isRegistered else { |
There was a problem hiding this comment.
Complete nit, but IMO if X is more readable than guard !X else
| guard !self.isRegistered else { | |
| if self.isRegistered { |
| while doing this. | ||
| 2. **Fulfill the operation's promise.** Succeed or fail the ``EventLoopPromise`` | ||
| that the caller handed to the ``ChannelCore`` method. | ||
| 3. **Fire the matching pipeline event.** Only now announce the new state to the |
There was a problem hiding this comment.
Minor: examples below use "broadcast", I think it's helpful to have consistent terminology here.
|
|
||
| First, if your activation is asynchronous — a `connect` that returns | ||
| `EINPROGRESS`, for example — you should hold on to the connect promise and only | ||
| fulfill it at the point where you actually become active. NIO stores it in a |
There was a problem hiding this comment.
nit: "you" don't become active, the channel does
| `pendingConnect` property and passes it into the activation path once the | ||
| connection has been established. Avoid succeeding the connect promise early and |
There was a problem hiding this comment.
I think it's worth being a bit more concrete here, "NIO stores it in a pendingConnect property..." sounds like NIO does this automatically for all channels which may be misleading if you are implementing a Channel which needs to support connect.
| is the one NIO uses for a full close, i.e. ``CloseMode/all``. The half-closure | ||
| modes are covered further down. | ||
|
|
||
| | Step | What happens | |
There was a problem hiding this comment.
nit: seems odd to use a table here instead of an ordered list
| | 5 | Succeed (or fail) the **close** promise | | ||
| | 6 | Fire ``ChannelInboundInvoker/fireChannelInactive()`` | | ||
| | 7 | Fire ``ChannelInboundInvoker/fireChannelUnregistered()`` | | ||
| | 8 | On a **later event loop tick**: remove all handlers from the pipeline, then succeed ``Channel/closeFuture`` | |
| // | ||
| // 2. Do the work and reconcile the state. Any error that we discover in | ||
| // here is recorded and only fired once the state is consistent again. | ||
| var errorCallouts: [(ChannelPipeline) -> Void] = [] |
There was a problem hiding this comment.
We only have at most one callout in this example: perhaps the array overcomplicates things here?
| guard !self.inputShutdown else { | ||
| promise?.fail(ChannelError.inputClosed) |
There was a problem hiding this comment.
same nit as about re: guard !x else
| private func flushNow() { | ||
| // Re-entrancy protection: a write issued from one of the callouts below is | ||
| // picked up by this loop, rather than starting a nested flush. | ||
| guard !self.inFlushNow else { return } |
Motivation:
The order in which a
Channelfulfills an operation's promise and fires the matching pipeline event is a guarantee handlers depend on, but it was only written down implicitly, spread across NIOPosix and ChannelNotificationTest. Anyone implementing ChannelCore had to reverse engineer it.Modifications:
Add Sources/NIOCore/Docs.docc/channel-notification-order.md, documenting the state -> promise -> pipeline event -> closeFuture ordering, with per-operation walkthroughs for registration, activation, closing, half-closure and writability, plus sections on error reporting, re-entrancy, guards and finally an short implementation checklist. Link it from the NIOCore article index.
Result:
ChannelCoreimplementers have a written reference for the notification order.