[GH-539] Implement no-flow-control extension #565
Conversation
gnodet
commented
Jul 26, 2024
gnodet
commented
- Support for new client side extension
- Implement no-flow-control extension (fixes Implement no-flow-control extension #539)
- Minor tweaks
There was a problem hiding this comment.
Several findings, see inline comments.
In general I wonder how much this brings in terms of throughput. The point of it all is to avoid that the channel remote window ever gets smaller than the packet size. I think we already have that. A connection would need to have a really huge latency to have the peer's window adjustments arrive so late that a sender would be blocked by a small channel window. Or the receiver would need to be much slower in processing data received than the sender sending it. In either case a sender would be blocked probably anyway by TCP/IP send or receive buffers being full.
How could we do an integration test? Is there a publicly available third-party SSH implementation besides OpenSSH that we could run in a container? AFAIK OpenSSH does not implement this extension.
| * connected peer also supports it. A value of {@code false} disables the {@code no-flow-control} completely, while | ||
| * the default {@code null} value, will support it, but not request it. | ||
| */ | ||
| public static final Property<Boolean> NO_FLOW_CONTROL = Property.bool(NoFlowControl.NAME); |
There was a problem hiding this comment.
Where should this be set? For a server it's OK, set it on the SshServer. But for the client side?
- On the
SshClient? That's not useful if one uses a singleSshClientfor multiple sessions; it would be advertised and might be activated for all sessions created through thatSshClient. - On the
ClientSession? That's way too late; a client session starts immediately when instantiated, so if the user code want to set it on the session, there'd be a race condition. The initial KEX might be on-going or already over, and the client might already have passed the point where it would send its SSH_MSG_EXT_INFO message.
| * Configuration value to enable {@code no-flow-control}. When set to {@code true}, the option will be used if the | ||
| * connected peer also supports it. A value of {@code false} disables the {@code no-flow-control} completely, while | ||
| * the default {@code null} value, will support it, but not request it. | ||
| */ |
There was a problem hiding this comment.
I don't like the mis-use of Boolean for a tri-state value, and I don't like the fact that null corresponds to 's' (supported).
Introduce an enum for this with two values: SUPPORTED and PREFERRED, and let null mean "do not advertise no-flow-control at all".
Additionally: under what condition exactly would it make sense for a server to ever send 'p' (PREFERRED)? A general-purpose server should probably never do so and at most send 's'(SUPPORTED).
For a server the default should be null (don't advertise it). Users who want to have a server that does implement this can set the property to SUPPORTED explicitly.
For a client sending 's' probably makes not much sense (unless there is a use case where having the server send 'p' would make sense). For a client it's either 'p' ("yes, I want this if you can do it, too"), or don't advertise it at all ("no, I don't want this, even if you can, because I might want to open multiple simultaneous channels").
| } | ||
|
|
||
| /** | ||
| * Read all incoming data and if END_FILE symbol is detected, kill client session to end test |
There was a problem hiding this comment.
Doesn't kill the client session; it closes the client channel (hard, at that). Why would the client do so? Wouldn't it be more natural to have the server close the channel (gracefully!) after it has sent the last data?
| public void setUp() throws Exception { | ||
| sshServer = setupTestServer(); | ||
|
|
||
| byte[] msg = IoUtils.toByteArray(getClass().getResourceAsStream("/big-msg.txt")); |
There was a problem hiding this comment.
Where is that resource? Why use a resource at all?
| * Test the no-flow-control extension. | ||
| */ | ||
| @FixMethodOrder(MethodSorters.NAME_ASCENDING) | ||
| public class NoFlowControlTest extends BaseTestSupport { |
There was a problem hiding this comment.
This test simulates a download. There should also be a test showing a file upload.
Plus: this needs tests with slow consumers, and it needs tests involving port forwarding with slow consumers. I'm a bit worried that no-flow-control may lead to a lot of data ending up being buffered somewhere. Especially if it's done like in this test: I suspect the "pending queue" will then get very large. That would be a no-go for a server, and might be a problem for a client.
If someone tunes a high latency connection by increasing send and receive buffer sizes, might we get into trouble? (Especially on a server. Consider a server with 1000 connections all having no-flow-control enabled and streaming lots of data.)
| // flow control is disabled, so just bail out | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Additionally, waitAndConsume() and waitForSpace()should also do an early return. The should not wait for anything. expand must not do anything.
| // flow control is disabled, so just bail out | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Additionally, release() must not do anything.
| @@ -94,6 +97,8 @@ protected void init(long size, long packetSize, PropertyResolver resolver) { | |||
| } | |||
|
|
|||
| synchronized (lock) { | |||
| Session session = channelInstance.getSession(); // this should only be null during tests | |||
| this.noFlowControl = session != null && session.isNoFlowControl(); | |||
There was a problem hiding this comment.
Below:
this.maxSize = this.noFlowControl ? Integer.MAX_VALUE : size;
this.packetSize = packetSize;
updateSize(this.maxSize);
| = ValidateUtils.checkInstanceOf(session, AbstractSession.class, "Not a supported session: %s", session); | ||
| abstractSession.activateNoFlowControl(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we avoid this code duplication between client & server?
| @@ -133,4 +151,39 @@ protected void handleServerSignatureAlgorithms(Session session, Collection<Strin | |||
| session.setSignatureFactories(clientAlgorithms); | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public void sendKexExtensions(Session session, KexPhase phase) throws Exception { | |||
There was a problem hiding this comment.
This must send something only if the server had announced ext-info-s. Otherwise the client must not send any extensions. See the logic in the DefaultServerKexExtensionHandler.
