Add closeOnDeinit to the NIOAsyncChannel init
#2592
Conversation
# Motivation In my previous PR, I already did the work to add `finishOnDeinit` configuration to the `NIOAsyncWriter` and `NIOAsyncSequenceProducer`. This PR also automatically migrated the `NIOAsyncChanell` to set the `finishOnDeinit = false`. This was intentional since we really want users to not use the deinit based cleanup; however, it also broke all current adopters of this API semantically and they might now run into the preconditions. # Modification This PR reverts the change in `NIOAsyncChannel` and does the usual deprecate + new init dance to provide users to configure this behaviour while still nudging them to check that this is really what they want. # Result Easier migration without semantically breaking current adopters of `NIOAsyncChannel`.
FranzBusch
force-pushed
the
fb-async-channel-deinit-based-cleanup
branch
from
cec1a4c to
1dbbec2
Compare
FranzBusch
changed the title
finishOnDeinit to the NIOAsyncChannel initcloseOnDeinit to the NIOAsyncChannel init
| public init( | ||
| synchronouslyWrapping channel: Channel, | ||
| configuration: Configuration = .init(), | ||
| closeOnDeinit: Bool |
There was a problem hiding this comment.
It's a shame we have to rely on users setting this to get the right behaviour because they can still set this to be true and get the old behaviour without a deprecation warning.
Ideally users shouldn't have to think about this and we would have two differently spelled inits, the deprecated old one and the new no-close-on-deinit one. That'd make it obvious which one to call.
I'm not sure what to suggest for naming here, however. A couple of ideas (don't love either):
- a
statichelperstatic func synchronouslyWrapping(_:configuration:) -> Self init(wrappingChannelSynchronously:configuration:)
There was a problem hiding this comment.
It's a shame we have to rely on users setting this to get the right behaviour because they can still set this to be true and get the old behaviour without a deprecation warning.
Yes.
Ideally users shouldn't have to think about this and we would have two differently spelled inits, the deprecated old one and the new no-close-on-deinit one. That'd make it obvious which one to call.
I think you're right
There was a problem hiding this comment.
I agree with those points. I had it quickly prototyped with a factory method but disliked it. Maybe just having a wrappingChannelSynchronously is the way out :D
|
@swift-server-bot test this please |
| /// - closeOnDeinit: Indicates if the underlying channel should be closed once the `inbound` and `outbound` have been deinited. We do not recommend to rely on | ||
| /// deinit based resource tear down. |
| /// - Parameters: | ||
| /// - channel: The ``Channel`` to wrap. | ||
| /// - configuration: The ``NIOAsyncChannel``s configuration. | ||
| /// - closeOnDeinit: Indicates if the underlying channel should be closed once the `inbound` and `outbound` have been deinited. We do not recommend to rely on |
FranzBusch
force-pushed
the
fb-async-channel-deinit-based-cleanup
branch
from
382cd83 to
9f09e39
Compare
|
@swift-server-bot test this please |
FranzBusch
force-pushed
the
fb-async-channel-deinit-based-cleanup
branch
from
9f09e39 to
148ba68
Compare
FranzBusch
force-pushed
the
fb-async-channel-deinit-based-cleanup
branch
from
148ba68 to
0f9d376
Compare
Motivation
In my previous PR, I already did the work to add
finishOnDeinitconfiguration to theNIOAsyncWriterandNIOAsyncSequenceProducer. This PR also automatically migrated theNIOAsyncChanellto set thefinishOnDeinit = false. This was intentional since we really want users to not use the deinit based cleanup; however, it also broke all current adopters of this API semantically and they might now run into the preconditions.Modification
This PR reverts the change in
NIOAsyncChanneland does the usual deprecate + new init dance to provide users to configure this behaviour while still nudging them to check that this is really what they want.Result
Easier migration without semantically breaking current adopters of
NIOAsyncChannel.