Finish multiplexer's inbound streams in more cases #483
Conversation
gjcairo
force-pushed
the
multiplexer-finish-inbound-streams
branch
from
a9113e2
to
92bb298
Compare
| for channel in self.streams.values { | ||
| channel.receiveStreamClosed(nil) | ||
| } | ||
|
|
||
| for channel in self._pendingStreams.values { | ||
| channel.receiveStreamClosed(nil) | ||
| } | ||
|
|
||
| self.streamChannelContinuation?.finish(throwing: error) | ||
| } | ||
|
|
||
| internal func propagateHandlerRemoved() { | ||
| for channel in self.streams.values { | ||
| channel.receiveStreamClosed(nil) | ||
| } | ||
|
|
||
| for channel in self._pendingStreams.values { | ||
| channel.receiveStreamClosed(nil) | ||
| } | ||
|
|
||
| self.streamChannelContinuation?.finish() | ||
| } |
There was a problem hiding this comment.
I don't think we should be closing the streams in these cases, we should just finish the continuation
There was a problem hiding this comment.
For errorCaught I think you're right (although I'm not sure what it would mean if there was a connection error fired down the pipeline but we keep the streams opened?); however if the handler was removed, wouldn't it make sense to close the streams? Is there anything we can do with them if the connection has died?
There was a problem hiding this comment.
In the HTTP2StreamMultiplexer, handler removed calls clearDidReadChannels in the common (i.e. this) multiplexer:
swift-nio-http2/Sources/NIOHTTP2/HTTP2StreamMultiplexer.swift
Lines 43 to 46 in 91136a6
I don't think we should have divergent behaviour between the two. When we reach handlerRemoved the streams will have already been closed.
For error caught, the HTTP2StreamMultiplexer only propagates stream errors to the child channels, they then get to decide how to act on those errors. Any connection level errors aren't propagated to streams and go down the channel pipeline and another handler decides how to act on it (usually by closing the connection, which will result in the streams being closed), but we should avoid short circuiting that because not all errors result in the connection being closed.
There was a problem hiding this comment.
Thanks George, that's clear. I'll make the change.
gjcairo
force-pushed
the
multiplexer-finish-inbound-streams
branch
from
9e1501a
to
f7be51f
Compare
Tests/NIOHTTP2Tests/SimpleClientServerInlineStreamMultiplexerTests.swift
Outdated
Show resolved
Hide resolved
…ests.swift Co-authored-by: George Barnett <gbarnett@apple.com>
Motivation
Currently, the multiplexer's inbound streams stream is finished only when the channel becomes inactive.
There are some scenarios in which the channel may be closed before it has a chance to become active, and the stream will never be finished. This can cause any users iterating over the stream to hang.
Modifications
This PR finishes the inbound streams stream when a connection error is fired, and when the handler is removed.
Result
Fewer bugs.