Don't crash on window adjust for a closed child channel#239
Open
andrewkomkov wants to merge 1 commit into
Open
Don't crash on window adjust for a closed child channel#239andrewkomkov wants to merge 1 commit into
andrewkomkov wants to merge 1 commit into
Conversation
A read delivered from the multiplexer's pending-read queue after local channel close triggers an outbound window adjust on a closed channel. The state machine used preconditionFailure, killing the whole process. The function already throws; surface a protocol violation instead and let the caller error the (already closing) channel.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ChildChannelStateMachine.sendChannelWindowAdjustcallspreconditionFailure("Sent channel window adjust on channel in invalid state")for theclosedLocally,closedRemotely, andclosedstates. SincepreconditionFailureis fatal, hitting any of those states crashes the entire process.This is reachable in normal operation. On a multiplexed session, a read can be delivered from the multiplexer's pending-read queue after the child channel has been closed locally (for example, the application cancels or closes a channel while reads are still in flight). Servicing that buffered read advances the inbound window and triggers an outbound window adjust, by which point the child channel state machine is already in a closed state, so the precondition fires and the whole process dies.
Stack summary
Fix
sendChannelWindowAdjustalreadythrows, so the closed states do not need to be fatal. ForclosedLocally,closedRemotely, andclosedwe now throwNIOSSHError.protocolViolationinstead of callingpreconditionFailure. The caller fails the associated promise and errors the channel, which is already closing, so the outcome is a clean, contained error rather than process death.requestedLocallyandrequestedRemotelykeep the existingpreconditionFailure, since reaching those states here genuinely indicates an internal logic error.Testing
swift buildsucceeds.swift testpasses (320 tests, 0 failures).I understand the Swift project may require a CLA; happy to sign it.