Don't fail closeFuture when an error occurs on closing #487
Conversation
| @@ -694,14 +694,14 @@ final class HTTP2StreamChannel: Channel, ChannelCore, @unchecked Sendable { | |||
| self.failPendingWrites(error: error) | |||
| if let promise = self.pendingClosePromise { | |||
| self.pendingClosePromise = nil | |||
| promise.fail(error) | |||
| promise.succeed() | |||
There was a problem hiding this comment.
I don't think this is what was agreed: IIRC the promise from close(mode:promise:) (i.e. pendingClosePromise) can fail to provide diagnostic information. It's the closeFuture (i.e. part of the Channel API) which mustn't fail.
There was a problem hiding this comment.
Agree with what @glbrntt said here. This means we need to change the implementation of executeThenClose to discard the error from close and instead add a call to closeFuture.get()
There was a problem hiding this comment.
And update the docs for Channel.closeFuture
There was a problem hiding this comment.
Oh okay, I misunderstood this. The one thing I found weird when implementing this (and why I decided to not fail this promise either) is that calling channel.close() will return a failed promise (since it creates one and passes it to channel.close(promise: newPromise) under the hood), which I personally find slightly confusing. But if you're okay with this behaviour I can just be clear in the docs about it.
There was a problem hiding this comment.
That's expected. Roughly speaking:
channel.close(promise:)(and variants of, i.e.channel.close()) are "close the channel and tell me how it closed" (cleanly/uncleanly)channel.closeFutureis "tell me when the channel is closed"
| let errorHandler = ErrorEncounteredHandler() | ||
| let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self) | ||
| clientMultiplexer.createStreamChannel(promise: streamChannelPromise) { channel in | ||
| try? channel.pipeline.addHandler(errorHandler).wait() |
There was a problem hiding this comment.
This stream channel initializer will be executed on the event-loop; you can't wait() while on an EL. You need to use an alternative API, like the sync operations.
There was a problem hiding this comment.
This one got marked as resolved but wasn't actually resolved.
FWIW I also don't think it's necessary for all of these tests to check this behaviour (i.e. the stream closed error gets fired down the pipeline); we end up with a much larger diff than necessary and harder to maintain tests. I'd prefer we had a separate test which asserts the specific behaviour.
There was a problem hiding this comment.
This one got marked as resolved but wasn't actually resolved.
Ah, sorry I fixed it in a bunch of places but missed a few others.
FWIW I also don't think it's necessary for all of these tests to check this behaviour [..] I'd prefer we had a separate test which asserts the specific behaviour.
We were already performing assertions on the close promise everywhere - you could use the same argument there. I thought that it was more consistent if we are going to assert something on the close promise that we also check the right behaviour is seen for the closeFuture and the errors being thrown downstream. But I can revert those changes if you disagree and add a single test.
There was a problem hiding this comment.
No worries, I saw the first one and assumed they'd all been missed.
I'm okay not reverting it, the work's been done now. I get the following the existing pattern argument; I just think we should be mindful when we make changes like this to keep tests maintainable. This package is bad enough as it is as most tests have about three different versions because of the different multiplexing APIs...
Lukasa
changed the title
| let errorHandler = ErrorEncounteredHandler() | ||
| let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self) | ||
| clientMultiplexer.createStreamChannel(promise: streamChannelPromise) { channel in | ||
| try? channel.pipeline.addHandler(errorHandler).wait() |
There was a problem hiding this comment.
This one got marked as resolved but wasn't actually resolved.
FWIW I also don't think it's necessary for all of these tests to check this behaviour (i.e. the stream closed error gets fired down the pipeline); we end up with a much larger diff than necessary and harder to maintain tests. I'd prefer we had a separate test which asserts the specific behaviour.
gjcairo
force-pushed
the
dont-fail-close-promise
branch
from
f2bbfc5 to
fb977b5
Compare
…s `executeThenClose` (#3032) After the changes introduced in apple/swift-nio-http2#487, we need to make a small change in the implementation of `NIOAsyncChannel` to wait on the `closeFuture` instead of on `close`'s promise in the `executeThenClose` implementation. ### Motivation: `executeThenClose` shouldn't fail from errors arising from closing the channel - at this point, the user of the channel cannot really do anything, and since the channel has been closed, we should not fail since resources have been cleaned up anyways. ### Modifications: This PR changes the implementation of `NIOAsyncChannel` to wait on the `closeFuture` instead of on `close`'s promise in the `executeThenClose` implementation. It also updates the docs for `closeFuture` to better explain when it will be succeeded and why it won't ever be failed. ### Result: `executeThenClose` won't throw errors upon closing.
Strictly speaking, a
closeoperation can't really fail (except when the closing mode isn't supported or the channel is already closed). Even if an error is encountered while closing a channel, or if an error caused the channel to be closed, the end result is the same: the channel will be closed.In this case in particular, when streams are closed from the client (i.e. by sending a RST_STREAM frame) the
HTTP2StreamChannelwould fail the channel'scloseFuture. This isn't appropriate however, as the channel is successfully closed.This change stops failing the stream channel's
closeFuture. Instead, if the close happens uncleanly, the error will be fired down the pipeline and theclosemethod's promise will be failed.