[SSHD-1173] Simplify writing in ChannelAsyncOutputStream #251
Conversation
This file contains 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
tomaswolf
force-pushed
the
sshd-1173
branch
9 times, most recently
from
21e0614
to
fd15c71
Compare
Remove the option not to send a chunk when the remote window is smaller than the packet size, and we're trying to write more than the remote window size bytes. This option may lead to hangs with peers that wait with sending their window adjustment until the window size is very low, or even zero. RFC 4254 does not mandate any particular point at which an SSH implementation should send a window adjustment. Implementations doing so only once the window has been fully consumed are well within the bounds of the specification. Therefore remove this option altogether, and always send some data if the window size is > 0. Refactor the writePacket() method to make it much simpler and clearer. Client options removed: * SftpModuleProperties.CHUNK_IF_WINDOW_LESS_THAN_PACKET * CoreModuleProperties.ASYNC_SERVER_STDOUT_CHUNK_BELOW_WINDOW_SIZE * CoreModuleProperties.ASYNC_SERVER_STDERR_CHUNK_BELOW_WINDOW_SIZE * ChannelAsyncOutputStream(Channel, byte, boolean) constructor These options were by default all false, potentially leading to hangs as described above. Already [SSHD-1123] had pointed out that problem, and bug [SSHD-1207] might also be caused by this. The feature was introduced in [SSHD-979] in commit e85b67e and later modified in commit 536d066. Also fix a race condition in WindowAdjustTest (unsynchronized access) that made the test frequently hang because the write queue maintained in that test got corrupted.
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.
Remove the option not to send a chunk when the remote window is smaller than the packet size, and we're trying to write more than the remote widow size bytes. This option may lead to hangs with peers that wait with sending their window adjustment until the window size is very low, or even zero.
RFC 4254 does not mandate any particular point at which an SSH implementation should send a window adjustment. Implementations doing so only once the window has been fully consumed are well within the bounds of the specification.
Therefore remove this option altogether, and always send some data if the window size is > 0. Refactor the writePacket() method to make it much simpler and clearer.
Client options removed:
These options were by default all false, potentially leading to hangs as described above. Already [SSHD-1123] had pointed out that problem, and bug [SSHD-1207] might also be caused by this. The feature was introduced in [SSHD-979] in commit e85b67e and later modified in commit 536d066.