lgoldstein commented on a change in pull request #163:
URL: https://github.com/apache/mina-sshd/pull/163#discussion_r484526909
##########
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:
This changes the behavior of the code in a significant way `put`
involves blocking the calling thread for an indeterminate amount of time:
>> Inserts the specified element into this queue, waiting if necessary for
space to become available.
whereas `add`
>> Inserts the specified element into this queue if it is possible to do so
immediately without violating capacity restrictions, returning {@code true}
upon success and throwing an {@code IllegalStateException} if no space is
currently available.
which in original code means that it never blocks since we do not place any
restriction. I would feel better if `offer` was used with a configurable
timeout (defined in `CoreModuleProperties`):
```java
Duration duration =
CoreModuleProperties.MAX_PENDING_FUTURE_ENQUEUE_WAIT.getRequired(this);
if (!writes.offer(future, duration.toMillis(), TimeUnit.MILLISECONDS)) {
throw ...;
}
```
A similar property should be used to initialize the queue so that users can
tailor the throughput limitation according to their needs.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
