[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #163: [SSHD-1070] Limit read-ahead in BufferedIoOutputStream

[GitBox <gi...@apache.org>]

tomaswolf commented on a change in pull request #163:
URL: https://github.com/apache/mina-sshd/pull/163#discussion_r484566434



##########
File path: sshd-core/src/main/java/org/apache/sshd/common/channel/BufferedIoOutputStream.java
##########
@@ -56,7 +56,12 @@ public IoWriteFuture writePacket(Buffer buffer) throws IOException {
         }
 
         IoWriteFutureImpl future = new IoWriteFutureImpl(getId(), buffer);
-        writes.add(future);
+        try {
+            writes.put(future);
+        } catch (InterruptedException e) {
+            Thread.interrupted();
+            throw new IOException(e);
+        }

Review comment:
       I know about the semantics of `put`. Yes, it'll block indefinitely until there is space again - which will be when the slow consumer has caught up. It doesn't make any sense to let the producer continue any earlier. But maybe there's a better place to implement this throttle than in this `BufferedIoOutputStream`.
   
   Your suggestion is a larger refactoring that I cannot do. Using `offer` with timeout one also would have to handle in the caller the case that the internal queue was full, and wait there, and there'd need to be a way to configure the queue size. Moreover the queue size and timeout needed would depend on the relative speeds of producer and consumer, which are unknown in general. I wouldn't know how to configure such a thing such that it worked for all cases. Just failing the transfer when the queue is ever full is IMO not an option; the whole mechanism should probably still work (albeit slowly) with a very fast producer and a very slow consumer.
   
   The problem is precisely that current code doesn't place any restrictions, and thus a fast producer can overrun a slow consumer so much that the server gets an OOME.
   
   But in any case SSHD-1070 and SSHD-1069 should probably be resolved together.
   
   BTW, `Thread.interrupted()`is wrong; should have been `Thread.currentThread().interrupt()`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org