-
Notifications
You must be signed in to change notification settings - Fork 189
Expose local SFTP window/packet size setters as public #1145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
b28b683
031cb04
e6afe07
dda4779
f2ee5a3
593e944
1e6fa26
9683e73
020a46b
11ba4fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,14 @@ public abstract class Channel { | |
|
|
||
| private static final AtomicInteger index = new AtomicInteger(); | ||
|
|
||
| // sanity cap for setLocalPacketSize: lmpsize is both used to allocate a Buffer of that size | ||
| // (e.g. ChannelSftp.start()) and advertised to the server as the max size of a single | ||
| // CHANNEL_DATA payload it may send us. That payload is wrapped in an SSH packet together with | ||
| // ~9 bytes of channel-data framing plus padding, so it must stay comfortably under | ||
| // Session.PACKET_MAX_SIZE (RFC 4253 6.1 Maximum Packet Length) or the resulting packet gets | ||
| // discarded by the transport layer, killing the connection. | ||
| private static final int MAX_LOCAL_PACKET_SIZE = Session.PACKET_MAX_SIZE - 4096; | ||
|
|
||
| int id; | ||
| volatile int recipient = -1; | ||
| protected byte[] type = Util.str2byte("foo"); | ||
|
|
@@ -406,16 +414,66 @@ synchronized void checkSpace(int len) throws IOException { | |
| } | ||
| } | ||
|
|
||
| void setLocalWindowSizeMax(int foo) { | ||
| this.lwsize_max = foo; | ||
| /** | ||
| * Sets the maximum local window size. | ||
| * | ||
| * <p> | ||
| * Both the maximum local window size and the initial local window size are set to the specified | ||
| * {@code size}. | ||
| * </p> | ||
| * | ||
| * @param size the maximum local window size in bytes | ||
| * @throws JSchException if the channel is already connected or if {@code size} is not positive | ||
| */ | ||
| public void setLocalWindowSizeMax(int size) throws JSchException { | ||
| if (isConnected()) { | ||
| throw new JSchException("local window size max cannot be changed after channel is connected"); | ||
| } | ||
| if (size <= 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should disallow setting lwsize_max to < lwsize?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact, I think we definitely should enforce that lwsize_max >= lwsize. |
||
| throw new JSchException("local window size max must be positive: " + size); | ||
| } | ||
| this.lwsize_max = size; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes me a bit nervous to allow users to set lwsize_max to an unbounded upper value.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sshj (net.schmizz.sshj) does not enforce an upper bound on window size.
Source: hierynomus/sshj, src/main/java/net/schmizz/sshj/connection/ConnectionImpl.java and src/main/java/net/schmizz/sshj/connection/channel/Window.java.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the information about sshj.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding checked JSchException vs IllegalArgumentException/IllegalStateException: since
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's go ahead and switch them to throwing checked JSchException's so that users are aware of the potential for an exception being thrown. |
||
| this.lwsize = size; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the maximum local window size. | ||
| * | ||
| * @return the maximum local window size in bytes | ||
| */ | ||
| public int getLocalWindowSizeMax() { | ||
| return this.lwsize_max; | ||
| } | ||
|
|
||
| void setLocalWindowSize(int foo) { | ||
| this.lwsize = foo; | ||
| } | ||
|
|
||
| void setLocalPacketSize(int foo) { | ||
| this.lmpsize = foo; | ||
| /** | ||
| * Sets the maximum local packet size. | ||
| * | ||
| * @param size the maximum local packet size in bytes | ||
| * @throws JSchException if the channel is already connected, if {@code size} is not positive, or | ||
| * if {@code size} exceeds the maximum allowed packet size | ||
| */ | ||
| public void setLocalPacketSize(int size) throws JSchException { | ||
| if (isConnected()) { | ||
| throw new JSchException("local packet size cannot be changed after channel is connected"); | ||
| } | ||
| if (size <= 0 || size > MAX_LOCAL_PACKET_SIZE) { | ||
| throw new JSchException("local packet size must be positive and not exceed " | ||
| + MAX_LOCAL_PACKET_SIZE + ": " + size); | ||
| } | ||
| this.lmpsize = size; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the maximum local packet size. | ||
| * | ||
| * @return the maximum local packet size in bytes | ||
| */ | ||
| public int getLocalPacketSize() { | ||
| return this.lmpsize; | ||
| } | ||
|
|
||
| synchronized void setRemoteWindowSize(long foo) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know what max values other implementations use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my current understanding, neither sshj nor OpenSSH caps local max packet size beyond the RFC 4253 transport packet-length ceiling: sshj's
setMaxPacketSizeonly rejects values<= 0(default 32KB, no upper bound), and OpenSSH hardcodesCHAN_SES_PACKET_DEFAULTat 32KB with no user-configurable option, while its ownPACKET_MAX_SIZEtransport ceiling is 256KB, the same value and name jsch uses.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to expose setting both the max packet size and max window size in order to obtain the performance increase you desire? Or is exposing just one or the other sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only
setLocalPacketSizeis strictly necessary: with the default 2MB window and 16 max pipelined requests, in-flight data stays well under the window even at 64KB packets, so the window increase shouldn't be required by that math. That said, we only benchmarked both changes together, not packet size alone, so this isn't empirically confirmed. However IMHO symmetry of setters should be present for parity's sake.