Expose local SFTP window/packet size setters as public by courville · Pull Request #1145 · mwiede/jsch

courville · 2026-09-07T17:12:51Z

ChannelSftp caps every pipelined SFTP READ request at a fixed 32KB local packet size (LOCAL_MAXIMUM_PACKET_SIZE), which limits transfer throughput even with multiple requests in flight. Channel already had setLocalWindowSizeMax/setLocalWindowSize/setLocalPacketSize setters that could raise these values per-channel before connecting, but they were package-private, so callers had no way to tune them.

Making them public lets consumers configure a larger packet/window size from their own code without having to fork the library default.

This enable to catch up throughput penalty and beat sshj implementation in my application nova video player cf. nova-video-player/aos-AVP#1943

ChannelSftp caps every pipelined SFTP READ request at a fixed 32KB local packet size (LOCAL_MAXIMUM_PACKET_SIZE), which limits transfer throughput even with multiple requests in flight. Channel already had setLocalWindowSizeMax/setLocalWindowSize/setLocalPacketSize setters that could raise these values per-channel before connecting, but they were package-private, so callers had no way to tune them. Making them public lets consumers configure a larger packet/window size from their own code without having to fork the library default.

Copilot

🟡 Changes recommended

The newly-public setters need argument/lifecycle validation (e.g., positive bounds and pre-connect-only) to avoid invalid SSH window/packet accounting and unsafe allocations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR exposes previously package-private Channel setters for local SSH channel window size and packet size, enabling consumers (notably ChannelSftp) to tune pipelined SFTP throughput by increasing per-channel flow-control limits before connecting.

Changes:

Made setLocalWindowSizeMax(int), setLocalWindowSize(int), and setLocalPacketSize(int) public on com.jcraft.jsch.Channel.

File summaries

File	Description
`src/main/java/com/jcraft/jsch/Channel.java`	Exposes local window/packet size configuration hooks to library consumers via public setters.

Review details

Files reviewed: 1/1 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+  public void setLocalWindowSizeMax(int foo) {
    this.lwsize_max = foo;
  }

-  void setLocalWindowSize(int foo) {
+  public void setLocalWindowSize(int foo) {
    this.lwsize = foo;
  }

-  void setLocalPacketSize(int foo) {
+  public void setLocalPacketSize(int foo) {
    this.lmpsize = foo;
  }


…etters Addresses upstream PR review feedback (mwiede#1145): the newly public setLocalWindowSizeMax/setLocalWindowSize/setLocalPacketSize accepted any int and could be called after connect(), which could break SSH window/packet accounting (invalid WINDOW_ADJUST deltas, lwsize > lwsize_max) or trigger unsafe buffer allocations. Each setter now rejects non-positive values and throws if the channel is already connected; setLocalWindowSize additionally rejects values exceeding the configured local window size max.

lmpsize is used to allocate a Buffer of that exact size (e.g. ChannelSftp.start()), so leaving it unbounded above zero still let callers request an unreasonably large allocation. Add a 16MB sanity ceiling, per further Copilot review feedback on mwiede#1145.

Replace the arbitrary 16MB sanity ceiling with a bound derived from Session.PACKET_MAX_SIZE (RFC 4253 6.1 Maximum Packet Length), the hard limit the transport layer already enforces on any incoming SSH packet. A CHANNEL_DATA reply carrying up to lmpsize bytes is wrapped in one SSH packet with framing/padding overhead, so lmpsize must stay safely under that limit; otherwise the server's response packet gets discarded by the transport layer, killing the connection outright (reproduced earlier with a 256KB lmpsize against a real OpenSSH 9.2 server). Session.PACKET_MAX_SIZE is made package-visible for this purpose.

CI's formatter-maven-plugin validate goal failed because the exception messages added in a previous commit exceeded the 100-column limit. Reformatted via formatter-maven-plugin format (eclipse-java-google-style.xml) to match.

norrisjeremy · 2026-09-08T12:08:46Z

Hi @courville,

These changes appear to cause numerous test failures.
Before we could even consider merging the changes, the reason for the test failures would need to be identified and resolved.

Thanks,
Jeremy

courville · 2026-09-08T12:10:48Z

Yes... fighting two conflicting review requirements. I will investigate but this is a pain for such a simple addition.

Session's own flow-control code was reusing the now strictly-validated public setLocalWindowSize to update the local window on every CHANNEL_DATA/CHANNEL_EXTENDED_DATA packet, but that path legitimately drives the window down to 0 and back up to lwsize_max post-connect, which the new pre-connect/positive-only validation rejected and broke transfers (surfaced as CI test failures). Add a package-private updateLocalWindowSize, mirroring the existing setRemoteWindowSize/addRemoteWindowSize split, so Session's internal bookkeeping has its own unrestricted (besides the lwsize_max bound) path while the public setter keeps its strict pre-connect contract.

norrisjeremy

Instead of throwing an unchecked IllegalArgumentException, could we make these methods instead throw a checked JSchException?
Or would throwing a checked exception force other changes to places that JSch internally calls these methods?

norrisjeremy · 2026-09-08T12:27:24Z

+    if (size <= 0) {
+      throw new IllegalArgumentException("local window size max must be positive: " + size);
+    }
+    this.lwsize_max = size;


It makes me a bit nervous to allow users to set lwsize_max to an unbounded upper value.
Do you know if other SSH implementations allow an unbound upper value or do they enforce a cap?

sshj (net.schmizz.sshj) does not enforce an upper bound on window size.

ConnectionImpl.setWindowSize(long windowSize): plain setter, no validation at all (not even a positive-value check). Default is 2048 * 1024 (2MB).

ConnectionImpl.setMaxPacketSize(int maxPacketSize): only checks maxPacketSize > 0; no upper cap. Default is 32 * 1024.

Window / Window.Local / Window.Remote constructors: only validate maxPacketSize > 0. The window size argument itself is not validated.

Source: hierynomus/sshj, src/main/java/net/schmizz/sshj/connection/ConnectionImpl.java and src/main/java/net/schmizz/sshj/connection/channel/Window.java.

Thank you for the information about sshj.
Do you know how implementations handle this, such as OpenSSH?

Regarding checked JSchException vs IllegalArgumentException/IllegalStateException: since
Session.run() now uses updateLocalWindowSize(), switching the public setters to throws JSchException
would not touch any internal call sites. I am happy to switch them to JSchException if you prefer
that for JSch's API conventions, or leave them as unchecked argument/state exceptions.

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.
Can you also add Javadocs to both these methods, being sure to highlight the reasons why a JSchException could be thrown too?

norrisjeremy · 2026-09-10T10:35:38Z

Instead of throwing an unchecked IllegalArgumentException, could we make these methods instead throw a checked JSchException? Or would throwing a checked exception force other changes to places that JSch internally calls these methods?

Hi @courville,

Did you have an opportunity to review the question above?

Thanks,
Jeremy

courville · 2026-09-10T14:08:40Z

To my current understanding, the answer depends on which point in the PR you compare against.

At the base of this PR, setLocalWindowSize was already called internally by Session.java (4 call sites, inside the packet-handling loop in run()), package-private and unvalidated. Switching it to a checked JSchException at that point would have forced changes to those internal call sites, since run() does not declare any checked exception and each call would need its own try/catch.

That dependency has since been removed within this PR: Session.java's internal call sites now use a separate package-private updateLocalWindowSize method instead, decoupling the internal flow-control bookkeeping from the public setters. As things stand now, no code in this repository calls setLocalWindowSizeMax, setLocalWindowSize, or setLocalPacketSize other than their own definitions in Channel.java, so switching these three to a checked JSchException would not require any further internal changes.

norrisjeremy · 2026-09-10T18:51:25Z

+  // ~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;


Do we know what max values other implementations use?

To my current understanding, neither sshj nor OpenSSH caps local max packet size beyond the RFC 4253 transport packet-length ceiling: sshj's setMaxPacketSize only rejects values <= 0 (default 32KB, no upper bound), and OpenSSH hardcodes CHAN_SES_PACKET_DEFAULT at 32KB with no user-configurable option, while its own PACKET_MAX_SIZE transport ceiling is 256KB, the same value and name jsch uses.

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?

Only setLocalPacketSize is 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.

norrisjeremy · 2026-09-14T14:31:20Z

+  // so keep this bookkeeping update package-private and unrestricted (besides the lwsize_max
+  // bound).
+  void updateLocalWindowSize(int size) {
+    if (size < 0 || size > lwsize_max) {


I'm mildly concerned about enforcing a non-negative size here. as JSch hasn't historically enforced that for the previous setLocalWindowSize() uses.
It appears JSch previously would have allowed a server to burst beyond the current lwsize (all the current uses simply call setLocalWindowSize(channel.lwsize - len), so if len exceeds lwsize, it could go negative).

Since the purpose of updateLocalWindowSize() is to for internal flow-control bookkeeping (as you mention in the comment), perhaps it would be best to simply not enforce any particular constraints on the value (instead trusting internal usages to do the right thing)?
I.e., just make updateLocalWindowSize() directly set the this.lwsize = foo; regardless of what value foo is?

All the changes to the PR were addressing the comments from sonarqube raised during the CI ... I think the original PR was much simpler.

I don't especially care if SonarQube raised it as an issue: we can't account for how random servers in the wild behave and I'm not willing to risk JSch suddenly closing connections because a random server sent a packet that exceeded the current window size.
Please change the updateLocalWindowSize() function exactly how setLocalWindowsSize() previously functioned (by just setting this.lwsize = foo; so that we don't have to worry about causing a regression for users.

Done. updateLocalWindowSize() has been simplified to directly assign this.lwsize = size; without any bounds checking, matching the exact prior behavior of setLocalWindowSize() in Session.run().

Restore previous direct assignment behavior for Session's flow-control bookkeeping in updateLocalWindowSize, avoiding any connection aborts if servers temporarily burst beyond the current window.

courville · 2026-09-14T17:08:26Z

If need be I can squash all back to simpler b28b683 (which was my original proposal and probably should have sticked to it...).

norrisjeremy · 2026-09-14T17:14:35Z

If need be I can squash all back to simpler b28b683 (which was my original proposal and probably should have sticked to it...).

No, I do agree that adding bounds checks to the publicly exposed versions of these methods as you've done is prudent, and switching the internal call sites to a non-bounds check version maintains compatibility without worry for introducing any possible regressions.

I think the only remaining changes are switching the public versions over to throwing checked JSchExceptions and adding Javadocs to them (in particular highlighting the conditions in which a JSchException could be thrown), this way users understand the API contract.

norrisjeremy · 2026-09-14T17:59:38Z

I wonder if we should add public getters in addition to publicly exposing the setters, so that users can successfully determine what the packet & window size values are?

norrisjeremy · 2026-09-14T18:01:55Z

+      throw new IllegalStateException(
+          "local window size max cannot be changed after channel is connected");
+    }
+    if (size <= 0) {


I wonder if we should disallow setting lwsize_max to < lwsize?

In fact, I think we definitely should enforce that lwsize_max >= lwsize.
Otherwise, in Session.java when we send a window update, we could end up sending an absurd value to the server (buf.putInt(channel.lwsize_max - channel.lwsize);).

…d add getters - Switch setLocalWindowSizeMax, setLocalWindowSize, and setLocalPacketSize to throw checked JSchException instead of unchecked exceptions. - Enforce size >= lwsize in setLocalWindowSizeMax to prevent invalid window adjust deltas. - Add public getLocalWindowSizeMax, getLocalWindowSize, and getLocalPacketSize getters. - Add comprehensive Javadocs describing parameters and exception conditions.

courville · 2026-09-14T19:07:12Z

All requested adjustments have been made:

Switched setLocalWindowSizeMax, setLocalWindowSize, and setLocalPacketSize to throw checked JSchException.
Enforced size >= lwsize in setLocalWindowSizeMax to avoid sending negative/underflow window adjust deltas to the server.
Added public getters: getLocalWindowSizeMax(), getLocalWindowSize(), and getLocalPacketSize().
Added Javadoc documentation to all new getters and setters detailing arguments and exact conditions triggering JSchException.

sonarqubecloud · 2026-09-14T19:07:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.8% Duplication on New Code

See analysis details on SonarQube Cloud

norrisjeremy · 2026-09-16T13:49:56Z

Hi @courville,

I've been giving this more thought and I think that allowing users to independently set lwsize + lwsize_max doesn't really make sense, because I can't think of a use case in which you want these values to be different when a new Channel starts.

I'm wondering if a better approach would be to only expose setLocalWindowSizeMax() and keep setLocalWindowSize() as an internal method that is just used for the internal bookkeeping like it is today.
And instead change setLocalWindowSizeMax() to set both lwsize & lwsize_max to the user supplied size value (with the constraints that the Channnel isn't connected and that the provided size value isn't <= 0).

What do you think?

Thanks,
Jeremy

Copilot AI lite review requested due to automatic review settings September 7, 2026 17:12

Copilot started reviewing on behalf of courville September 7, 2026 17:13 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

courville added 4 commits September 7, 2026 20:41

Apply Google Java Format to the new setter validation

f2ee5a3

CI's formatter-maven-plugin validate goal failed because the exception messages added in a previous commit exceeded the 100-column limit. Reformatted via formatter-maven-plugin format (eclipse-java-google-style.xml) to match.

norrisjeremy reviewed Sep 8, 2026

View reviewed changes

norrisjeremy reviewed Sep 10, 2026

View reviewed changes

norrisjeremy reviewed Sep 14, 2026

View reviewed changes

Remove bounds check from internal updateLocalWindowSize

1e6fa26

Restore previous direct assignment behavior for Session's flow-control bookkeeping in updateLocalWindowSize, avoiding any connection aborts if servers temporarily burst beyond the current window.

norrisjeremy reviewed Sep 14, 2026

View reviewed changes

Conversation

courville commented Sep 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

norrisjeremy commented Sep 8, 2026

Uh oh!

courville commented Sep 8, 2026

Uh oh!

norrisjeremy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

norrisjeremy commented Sep 10, 2026

Uh oh!

courville commented Sep 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

courville Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

courville commented Sep 14, 2026

Uh oh!

norrisjeremy commented Sep 14, 2026

Uh oh!

norrisjeremy commented Sep 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

norrisjeremy Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

courville commented Sep 14, 2026

Uh oh!

sonarqubecloud Bot commented Sep 14, 2026

Quality Gate passed

Uh oh!

norrisjeremy commented Sep 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

norrisjeremy left a comment •

edited

Loading

courville Sep 14, 2026 •

edited

Loading

norrisjeremy Sep 14, 2026 •

edited

Loading