-
Notifications
You must be signed in to change notification settings - Fork 644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Retain a ref to NIOAsyncWriter until channel active #2703
Conversation
Motivation: NIOAsyncChannel requires users to explicitly close it, this is typically done by calling `executeThenClose`. If a `NIOAsyncChannel` isn't closed then its outbound writer will hit a precondition failure on `deinit`. Not calling `executeThenClose` is a programmer error. However there are some sharp edges: if NIO never returns the `NIOAsyncChannel` to the caller (e.g. if a connect attempt fails) then nothing will finish the writer and precondition will fail in the deinit. Working around this from a user perspective is non-obvious and requires keep tracking of all `NIOAsyncChannel`s created from a connect attempt and closing the unused ones. We still want to maintain the precondition when users don't close the channel, one way of achieving this is by defining a point in time at which NIO hands responsibility of the channel to the user. Modifications: - Retain the writer in the outbound writer handler until channel active - On successful connect attempts, the channel becomes active and the connected channel is returned to the caller. - On failed attempts channel active isn't called so the writer is retained until the handler is removed from the pipeline at which point it is finished. Result: Failed connect attempts don't result in precondition failures when using NIOAsyncChannel.
Hmm, CI is failing to install jemalloc
|
@swift-server-bot test this please |
I'm happy with this, but would like JW to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! This seems like a good targeted fix here and for the connect
case makes total sense to me.
Now we have a fun mixture of failures unrelated to the changes... Swift 5.9 and 5.10 are failing some datagram tests:
and
Swift 5.9 cxx interop is failing to establish connections to archive.ubuntu.com to install various dependencies:
|
This likely explains why various dependencies couldn't be installed: https://status.canonical.com/#/incident/KNms6QK9ewuzz-7xUsPsNylV20jEt5kyKsd8A-3ptQHensLmCBVh6VphXOOE4t72EY4o3wVOetg9awfJ1hij3w== |
@swift-server-bot test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thank you!
@swift-server-bot test this please |
Motivation:
NIOAsyncChannel requires users to explicitly close it, this is typically done by calling
executeThenClose
. If aNIOAsyncChannel
isn't closed then its outbound writer will hit a precondition failure ondeinit
. Not callingexecuteThenClose
is a programmer error.However there are some sharp edges: if NIO never returns the
NIOAsyncChannel
to the caller (e.g. if a connect attempt fails) then nothing will finish the writer and precondition will fail in the deinit. Working around this from a user perspective is non-obvious and requires keep tracking of allNIOAsyncChannel
s created from a connect attempt and closing the unused ones.We still want to maintain the precondition when users don't close the channel, one way of achieving this is by defining a point in time at which NIO hands responsibility of the channel to the user.
Modifications:
Result:
Failed connect attempts don't result in precondition failures when using NIOAsyncChannel.