Fix ReadAheadRemoteFileInputStream not reading the whole file if a buffer is too big #769
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
…ffer is too big If an instance of ReadAheadRemoteFileInputStream before this change is wrapped into a BufferedInputStream with a big buffer, the SSH client requests big packets from the server. It turned out that if the server had sent a response smaller than requested, the client wouldn't have adjusted to decreased window size, and would have read the file incorrectly. This change detects cases when the server is not able to fulfil client's requests. Since this change, the client adjusts the maximum request length, sends new read-ahead requests, and starts to ignore all read-ahead requests sent earlier. Just specifying some allegedly small constant buffer size wouldn't have helped in all possible cases. There is no way to explicitly get the maximum request length inside a client. All that limits differ from server to server. For instance, OpenSSH defines SFTP_MAX_MSG_LENGTH as 256 * 1024. Apache SSHD defines MAX_READDATA_PACKET_LENGTH as 63 * 1024, and it allows to redefine that size. Interestingly, a similar issue hierynomus#183 was fixed many years ago, but the bug was actually in the code introduced for that fix.
Codecov Report
@@ Coverage Diff @@
## master #769 +/- ##
============================================
+ Coverage 63.93% 64.37% +0.44%
- Complexity 1452 1458 +6
============================================
Files 211 211
Lines 8617 8615 -2
Branches 811 806 -5
============================================
+ Hits 5509 5546 +37
+ Misses 2677 2645 -32
+ Partials 431 424 -7
Continue to review full report at Codecov.
|
|
After writing this simple piece of code, I stumbled upon questions: why do I remove already received data chunks if the window is adjusted? Why not reuse them later? The answer came after few hours of hacking. Indeed, it would be cool to make something as complicated as an interval tree. However, if we don't remove the data chunks which can be used only after many other smaller requests, the size of the unconfirmed reads will be equal to |
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
|
|
||
| private final byte[] b = new byte[1]; | ||
|
|
||
| private final int maxUnconfirmedReads; | ||
| private final long readAheadLimit; | ||
| private final Queue<Promise<Response, SFTPException>> unconfirmedReads = new LinkedList<Promise<Response, SFTPException>>(); | ||
| private final Queue<Long> unconfirmedReadOffsets = new LinkedList<Long>(); | ||
| private final Queue<UnconfirmedRead> unconfirmedReads = new LinkedList<>(); |
There was a problem hiding this comment.
JdkObsolete: It is very rare for LinkedList to out-perform ArrayList or ArrayDeque. Avoid it unless you're willing to invest a lot of time into benchmarking. Caveat: LinkedList supports null elements, but ArrayDeque does not. (details)
(at-me in a reply with help or ignore)
| this.readAheadLimit = readAheadLimit > 0 ? fileOffset + readAheadLimit : Long.MAX_VALUE; | ||
| } | ||
|
|
||
| private ByteArrayInputStream pending = new ByteArrayInputStream(new byte[0]); | ||
|
|
||
| private boolean retrieveUnconfirmedRead(boolean blocking) throws IOException { | ||
| if (unconfirmedReads.size() <= 0) { | ||
| final UnconfirmedRead unconfirmedRead = unconfirmedReads.peek(); | ||
| if (unconfirmedRead == null || !blocking && !unconfirmedRead.getPromise().isDelivered()) { |
There was a problem hiding this comment.
OperatorPrecedence: Use grouping parenthesis to make the operator precedence explicit (details)
(at-me in a reply with help or ignore)
hierynomus
approved these changes
Mar 4, 2022
|
Thanks for the PR @vladimirlagunov. High quality as always! |
vladimirlagunov
added a commit
to vladimirlagunov/sshj
that referenced
this pull request
Apr 6, 2022
…oteFile Due to a bug in logic introduced by hierynomus#769, RemoteFile.ReadAheadRemoteFileInputStream started to send new read ahead requests for file parts that had already been requested. Every call to read() asked the server to send parts of the file from the point which is already downloaded. Instead, it should have asked to send parts after the last requested part. This commit adds exactly this logic. The bug didn't cause content corruption. It only affected performance, both on servers and on clients.
vladimirlagunov
added a commit
to vladimirlagunov/sshj
that referenced
this pull request
Apr 6, 2022
…oteFile Due to a bug in logic introduced by hierynomus#769, RemoteFile.ReadAheadRemoteFileInputStream started to send new read ahead requests for file parts that had already been requested. Every call to read() asked the server to send parts of the file from the point which is already downloaded. Instead, it should have asked to send parts after the last requested part. This commit adds exactly this logic. The bug didn't cause content corruption. It only affected performance, both on servers and on clients.
hierynomus
pushed a commit
that referenced
this pull request
Apr 7, 2022
) Due to a bug in logic introduced by #769, RemoteFile.ReadAheadRemoteFileInputStream started to send new read ahead requests for file parts that had already been requested. Every call to read() asked the server to send parts of the file from the point which is already downloaded. Instead, it should have asked to send parts after the last requested part. This commit adds exactly this logic. The bug didn't cause content corruption. It only affected performance, both on servers and on clients.
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.
If an instance of ReadAheadRemoteFileInputStream before this change is wrapped into a BufferedInputStream with a big buffer, the SSH client requests big packets from the server. It turned out that if the server had sent a response smaller than requested, the client wouldn't have adjusted to decreased window size, and would have read the file incorrectly.
This change detects cases when the server is not able to fulfil client's requests. Since this change, the client adjusts the maximum request length, sends new read-ahead requests, and starts to ignore all read-ahead requests sent earlier.
Just specifying some allegedly small constant buffer size wouldn't have helped in all possible cases. There is no way to explicitly get the maximum request length inside a client. All that limits differ from server to server. For instance, OpenSSH defines SFTP_MAX_MSG_LENGTH as 256 * 1024. Apache SSHD defines MAX_READDATA_PACKET_LENGTH as 63 * 1024, and it allows to redefine that size.
Interestingly, a similar issue #183 was fixed many years ago, but the bug was actually in the code introduced for that fix.