Wait for closeFuture instead of close promise in NIOAsyncChannel's executeThenClose
#3032
Conversation
gjcairo
force-pushed
the
async-channel-executethenclose
branch
from
f3ed6fe to
d050010
Compare
| // at this point. | ||
| self.channel.close(promise: nil) | ||
| // `closeFuture` is never failed, so we can ignore the error | ||
| try? await self.channel.closeFuture.get() |
There was a problem hiding this comment.
Not sure whether we decided to ignore closeFuture errors entirely or not – it seems completely reasonable to ignore them though.
Suspect @FranzBusch has opinions.
There was a problem hiding this comment.
This future should never be failed; if we're considering a failing closeFuture the result of a bad implementation of a Channel but not something that should happen in any other scenario, it seemed reasonable to me to ignore all errors.
There was a problem hiding this comment.
The original goals were:
- Make sure we only excited from
executeThenClosewhen the channel is actually closed - Surface any errors if closing fails
Now 1. is still being upheld here since we wait for the closeFuture. It also turns out that closing can't fail and a call to close() must always result in the closeFuture completing. Strictly speaking succeeding actually.
Now we have two options here. We can just assume that the "strict" part is true so there will never be any errors. However, there might be some code out there that actually fails the close future. What if we catch the error and add an assert that triggers if this close future actually fails to help us spot incorrect implementations?
There was a problem hiding this comment.
I agree, it just depends whether we want to help flush out the different implementations or not.
There was a problem hiding this comment.
Fair enough, adding an assertion sounds useful.
After the changes introduced in apple/swift-nio-http2#487, we need to make a small change in the implementation of
NIOAsyncChannelto wait on thecloseFutureinstead of onclose's promise in theexecuteThenCloseimplementation.Motivation:
executeThenCloseshouldn'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
NIOAsyncChannelto wait on thecloseFutureinstead of onclose's promise in theexecuteThenCloseimplementation.It also updates the docs for
closeFutureto better explain when it will be succeeded and why it won't ever be failed.Result:
executeThenClosewon't throw errors upon closing.