You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by GitBox <gi...@apache.org> on 2020/09/14 08:14:02 UTC
[GitHub] [mina-sshd] gnodet opened a new pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
gnodet opened a new pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166
…rding channels
----------------------------------------------------------------
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
[GitHub] [mina-sshd] fengjiajie commented on pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
fengjiajie commented on pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#issuecomment-694755847
I have tried it,
ut failed, 45f84aab5 [SSHD-1003] Use asynchronous streams when forwarding ports
ut success, 449c2a56c (prev SSHD-1003) [SSHD-1009] SSHD SCP does not work with WinSCP
ut failed, e512f68 [SSHD-1028] Fix SSH_MSG_DISCONNECT: Too many concurrent connections
ut failed, e040cb4dd0aad8 (prev SSHD-1028) [SSHD-1038] Refactor packages from a module into a cleaner hierarchy
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet commented on pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#issuecomment-695931866
@fengjiajie yes, unfortunately, the test was missing some assumptions. I've seen it pass when there were errors in the transmission. I've fixed the test in master.
However, the result is that I can't make it pass since https://github.com/apache/mina-sshd/commit/45f84aab59b2e11d72942cffe9d810e37ab64959 so I'm thinking about somehow reverting this commit and enabling those tests by default in a first pass. It seems the problem comes from the {{ChannelAsyncOutputStream}}, but I haven't been able to find out where.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] fengjiajie commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
fengjiajie commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r487798281
##########
File path: sshd-core/src/main/java/org/apache/sshd/server/forward/TcpipServerChannel.java
##########
@@ -217,9 +225,23 @@ public void messageReceived(IoSession session, Readable message) throws Exceptio
log.debug("doInit({}) Ignoring write to channel in CLOSING state", TcpipServerChannel.this);
}
} else {
- Buffer buffer = new ByteArrayBuffer(message.available(), false);
+ int length = message.available();
+ Buffer buffer = new ByteArrayBuffer(length, false);
buffer.putBuffer(message);
- out.writePacket(buffer);
+ long total = inFlightDataSize.addAndGet(length);
+ if (total > maxBufferSize) {
+ session.suspendRead();
+ }
+ IoWriteFuture ioWriteFuture = out.writePacket(buffer);
+ ioWriteFuture.addListener(new SshFutureListener<IoWriteFuture>() {
+ @Override
+ public void operationComplete(IoWriteFuture future) {
+ long total = inFlightDataSize.addAndGet(-length);
+ if (total <= maxBufferSize) {
Review comment:
Maybe this threshold can be configured?
when maxBufferSize = 1MB, I think ```if (total <= 500KB)``` might be better for performance? to avoid frequent suspensions and resumptions
inFlightDataSize > 1 MB, suspendRead
inFlightDataSize < 0.5 MB, resumedRead
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r488009388
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/io/nio2/Nio2Session.java
##########
@@ -382,7 +385,36 @@ protected void handleReadCycleFailure(ByteBuffer buffer, Readable bufReader, Thr
exceptionCaught(exc);
}
+ @Override
+ public void suspendRead() {
+ log.debug("suspendRead({})", this);
+ suspendRead.set(true);
+ }
+
+ @Override
+ public void resumeRead() {
+ Runnable runnable = readRunnable.getAndSet(null);
+ if (runnable != null) {
+ log.debug("resumeRead({})", this);
+ suspendRead.set(false);
Review comment:
I don't think this will change the behavior, but it will be cleaner.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] fengjiajie commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
fengjiajie commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r488451365
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/io/nio2/Nio2Session.java
##########
@@ -70,6 +71,8 @@
private final AtomicLong lastReadCycleStart = new AtomicLong();
private final AtomicLong writeCyclesCounter = new AtomicLong();
private final AtomicLong lastWriteCycleStart = new AtomicLong();
+ private final AtomicBoolean suspendRead = new AtomicBoolean(false);
+ private final AtomicReference<Runnable> readRunnable = new AtomicReference<>(null);
Review comment:
Does it need to be processed in close() ?
I feel like it might not be necessary, but not really sure.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r490244112
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/io/nio2/Nio2Session.java
##########
@@ -382,7 +385,32 @@ protected void handleReadCycleFailure(ByteBuffer buffer, Readable bufReader, Thr
exceptionCaught(exc);
}
+ @Override
+ public void suspendRead() {
+ log.trace("suspendRead({})", this);
+ if (suspendRead.compareAndSet(false, true)) {
+ log.debug("suspendRead({}) requesting read suspension", this);
+ }
+ }
+
+ @Override
+ public void resumeRead() {
+ log.trace("resumeRead({})", this);
+ Runnable runnable = readRunnable.getAndSet(null);
+ suspendRead.set(false);
+ if (runnable != null) {
+ log.debug("resumeRead({}) resuming read", this);
+ runnable.run();
+ }
+ }
+
protected void doReadCycle(ByteBuffer buffer, Nio2CompletionHandler<Integer, Object> completion) {
+ if (suspendRead.get()) {
+ log.debug("doReadCycle({}) suspending reading", this);
+ readRunnable.set(() -> doReadCycle(buffer, completion));
+ return;
+ }
+
Review comment:
Nice catch, i've pushed a new commit with a synchronized block to avoid that.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet merged pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet merged pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166
----------------------------------------------------------------
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
[GitHub] [mina-sshd] fengjiajie commented on pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
fengjiajie commented on pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#issuecomment-694660744
@gnodet Hi, I don't know how to construct this unit test now, so I had to look at how unit tests have been implemented before, then I found `PortForwardingLoadTest.testLocalForwardingPayload` , but it always fails to run on my computer, I'm looking at why it failed.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r489990336
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/io/nio2/Nio2Session.java
##########
@@ -382,7 +385,32 @@ protected void handleReadCycleFailure(ByteBuffer buffer, Readable bufReader, Thr
exceptionCaught(exc);
}
+ @Override
+ public void suspendRead() {
+ log.trace("suspendRead({})", this);
+ if (suspendRead.compareAndSet(false, true)) {
+ log.debug("suspendRead({}) requesting read suspension", this);
+ }
+ }
+
+ @Override
+ public void resumeRead() {
+ log.trace("resumeRead({})", this);
+ Runnable runnable = readRunnable.getAndSet(null);
+ suspendRead.set(false);
+ if (runnable != null) {
+ log.debug("resumeRead({}) resuming read", this);
+ runnable.run();
+ }
+ }
+
protected void doReadCycle(ByteBuffer buffer, Nio2CompletionHandler<Integer, Object> completion) {
+ if (suspendRead.get()) {
+ log.debug("doReadCycle({}) suspending reading", this);
+ readRunnable.set(() -> doReadCycle(buffer, completion));
+ return;
+ }
+
Review comment:
Seems to me that there is a chance of a race condition as follows:
* Thread A calls `suspendRead` and marks `suspendedRead` as _true_
* Thread A is pre-empted by thread B that enters `doReadCycle`
* Thread B sees that `suspendRead` is _true_ and enters the `if` block, but is preempted by thread A before calling`readRunnable.set(...)`
* Thread A calls `resumeRead` - calls `readRunnable.getAndSet(null);` and gets _null_ - so thinks there is nothing to do
* Thread B resumes its execution and calls `.set(....)` but no one will call the set _Runnable_
Result: reading is not resumed.
I think the suspend/resume should have some mutual exclusion mechanism to prevent this.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet edited a comment on pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet edited a comment on pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#issuecomment-695931866
@fengjiajie yes, unfortunately, the test was missing some assumptions. I've seen it pass when there were errors in the transmission. I've fixed the test in master.
However, the result is that I can't make it pass since https://github.com/apache/mina-sshd/commit/45f84aab59b2e11d72942cffe9d810e37ab64959 so I'm thinking about somehow reverting this commit and enabling those tests by default in a first pass. It seems the problem comes from the `ChannelAsyncOutputStream`, but I haven't been able to find out where.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r490246206
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/io/nio2/Nio2Session.java
##########
@@ -382,7 +385,32 @@ protected void handleReadCycleFailure(ByteBuffer buffer, Readable bufReader, Thr
exceptionCaught(exc);
}
+ @Override
+ public void suspendRead() {
+ log.trace("suspendRead({})", this);
+ if (suspendRead.compareAndSet(false, true)) {
+ log.debug("suspendRead({}) requesting read suspension", this);
+ }
+ }
+
+ @Override
+ public void resumeRead() {
+ log.trace("resumeRead({})", this);
+ Runnable runnable = readRunnable.getAndSet(null);
+ suspendRead.set(false);
+ if (runnable != null) {
+ log.debug("resumeRead({}) resuming read", this);
+ runnable.run();
+ }
+ }
+
protected void doReadCycle(ByteBuffer buffer, Nio2CompletionHandler<Integer, Object> completion) {
+ if (suspendRead.get()) {
+ log.debug("doReadCycle({}) suspending reading", this);
+ readRunnable.set(() -> doReadCycle(buffer, completion));
+ return;
+ }
+
Review comment:
Looks good to me...
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r488594986
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/io/nio2/Nio2Session.java
##########
@@ -70,6 +71,8 @@
private final AtomicLong lastReadCycleStart = new AtomicLong();
private final AtomicLong writeCyclesCounter = new AtomicLong();
private final AtomicLong lastWriteCycleStart = new AtomicLong();
+ private final AtomicBoolean suspendRead = new AtomicBoolean(false);
+ private final AtomicReference<Runnable> readRunnable = new AtomicReference<>(null);
Review comment:
In the current state, it's not needed imho, because this is only used for "dumb" tcp connections, i.e. there's no graceful shutdown of the sessions, the sockets are simply closed for forwarding sessions. I've just added some javadoc to make it clear.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet edited a comment on pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet edited a comment on pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#issuecomment-695931866
@fengjiajie yes, unfortunately, the test was missing some assumptions. I've seen it pass when there were errors in the transmission. I've fixed the test in master.
However, the result is that I can't make it pass since https://github.com/apache/mina-sshd/commit/45f84aab59b2e11d72942cffe9d810e37ab64959 so I'm thinking about somehow reverting this commit and enabling those tests by default in a first pass. It seems the problem comes from the `ChannelAsyncOutputStream`, but I haven't been able to find out where.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r489990336
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/io/nio2/Nio2Session.java
##########
@@ -382,7 +385,32 @@ protected void handleReadCycleFailure(ByteBuffer buffer, Readable bufReader, Thr
exceptionCaught(exc);
}
+ @Override
+ public void suspendRead() {
+ log.trace("suspendRead({})", this);
+ if (suspendRead.compareAndSet(false, true)) {
+ log.debug("suspendRead({}) requesting read suspension", this);
+ }
+ }
+
+ @Override
+ public void resumeRead() {
+ log.trace("resumeRead({})", this);
+ Runnable runnable = readRunnable.getAndSet(null);
+ suspendRead.set(false);
+ if (runnable != null) {
+ log.debug("resumeRead({}) resuming read", this);
+ runnable.run();
+ }
+ }
+
protected void doReadCycle(ByteBuffer buffer, Nio2CompletionHandler<Integer, Object> completion) {
+ if (suspendRead.get()) {
+ log.debug("doReadCycle({}) suspending reading", this);
+ readRunnable.set(() -> doReadCycle(buffer, completion));
+ return;
+ }
+
Review comment:
Seems to me that there is a chance of a race condition as follows:
* Thread A calls `suspendRead` and marks `suspendedRead` as _true_
* Thread A is pre-empted by thread B that enters `doReadCycle`
* Thread B sees that `suspendRead` is _true_ and enters the `if` block, but is preempted by thread A before calling`readRunnable.set(...)`
* Thread A calls `resumeRead` - calls `readRunnable.getAndSet(null);` and gets _null_ - so thinks there is nothing to do
* Thread B resumes its execution and calls `.set(....)`
Result: reading is not resumed.
I think the suspend/resume should have some mutual exclusion mechanism to prevent this.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet commented on pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#issuecomment-695931866
@fengjiajie yes, unfortunately, the test was missing some assumptions. I've seen it pass when there were errors in the transmission. I've fixed the test in master.
However, the result is that I can't make it pass since https://github.com/apache/mina-sshd/commit/45f84aab59b2e11d72942cffe9d810e37ab64959 so I'm thinking about somehow reverting this commit and enabling those tests by default in a first pass. It seems the problem comes from the {{ChannelAsyncOutputStream}}, but I haven't been able to find out where.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r487996932
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/io/nio2/Nio2Session.java
##########
@@ -382,7 +385,36 @@ protected void handleReadCycleFailure(ByteBuffer buffer, Readable bufReader, Thr
exceptionCaught(exc);
}
+ @Override
+ public void suspendRead() {
+ log.debug("suspendRead({})", this);
+ suspendRead.set(true);
+ }
+
+ @Override
+ public void resumeRead() {
+ Runnable runnable = readRunnable.getAndSet(null);
+ if (runnable != null) {
+ log.debug("resumeRead({})", this);
+ suspendRead.set(false);
Review comment:
Shouldn't this be outside the "if", between lines 396/397?
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet commented on pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#issuecomment-694284129
@fengjiajie I'm still waiting for a test before I can commit this PR. Do you think you could come up with a unit test for the issue ?
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet merged pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet merged pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166
----------------------------------------------------------------
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
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r489990862
##########
File path: sshd-core/src/main/java/org/apache/sshd/server/forward/TcpipServerChannel.java
##########
@@ -59,6 +62,9 @@
import org.apache.sshd.server.channel.AbstractServerChannel;
import org.apache.sshd.server.forward.TcpForwardingFilter.Type;
+import static org.apache.sshd.core.CoreModuleProperties.TCPIP_SERVER_CHANNEL_BUFFER_SIZE_THRESHOLD_HIGH;
+import static org.apache.sshd.core.CoreModuleProperties.TCPIP_SERVER_CHANNEL_BUFFER_SIZE_THRESHOLD_LOW;
+
Review comment:
Doesn't _checkstyle_ flag static imports as errors ?
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#discussion_r487826081
##########
File path: sshd-core/src/main/java/org/apache/sshd/server/forward/TcpipServerChannel.java
##########
@@ -217,9 +225,23 @@ public void messageReceived(IoSession session, Readable message) throws Exceptio
log.debug("doInit({}) Ignoring write to channel in CLOSING state", TcpipServerChannel.this);
}
} else {
- Buffer buffer = new ByteArrayBuffer(message.available(), false);
+ int length = message.available();
+ Buffer buffer = new ByteArrayBuffer(length, false);
buffer.putBuffer(message);
- out.writePacket(buffer);
+ long total = inFlightDataSize.addAndGet(length);
+ if (total > maxBufferSize) {
+ session.suspendRead();
+ }
+ IoWriteFuture ioWriteFuture = out.writePacket(buffer);
+ ioWriteFuture.addListener(new SshFutureListener<IoWriteFuture>() {
+ @Override
+ public void operationComplete(IoWriteFuture future) {
+ long total = inFlightDataSize.addAndGet(-length);
+ if (total <= maxBufferSize) {
Review comment:
Right, that makes sense, I'll split the property.
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet commented on pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#issuecomment-694687259
@fengjiajie it seems this test is failing since [SSHD-1003](https://issues.apache.org/jira/browse/SSHD-1003).
----------------------------------------------------------------
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
[GitHub] [mina-sshd] gnodet commented on pull request #166: [SSHD-1070] Limit the amount of data that is kept in memory for forwa…
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #166:
URL: https://github.com/apache/mina-sshd/pull/166#issuecomment-694707083
Not really. In fact, the test was working better before the partial revert of SSHD-1003 in https://github.com/apache/mina-sshd/commit/e512f68e2067723ce85c0f8e6a8642f2773aae9c
----------------------------------------------------------------
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