[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #123: SFTP transfer speeds, clean API
lgoldstein commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r412199074
##
File path:
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##
@@ -954,7 +954,9 @@ protected IoWriteFuture doWritePacket(Buffer buffer) throws
IOException {
synchronized (encodeLock) {
Buffer packet = resolveOutputPacket(buffer);
IoSession networkSession = getIoSession();
-return networkSession.writePacket(packet);
+IoWriteFuture future = networkSession.writePacket(packet);
+buffer.rpos(buffer.wpos());
+return future;
Review comment:
>> Ok, so the problem is in the assumptions about the writePacket.
I did not mean that there is any problem with the call since the packet is
obviously a different instance than the original buffer. If there is some
similar optimization that needs to be done on the packet then indeed we need to
make it **after** being informed (via a listener) that the packet has been
consumed.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #123: SFTP transfer speeds, clean API
lgoldstein commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r412195385
##
File path:
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##
@@ -954,7 +954,9 @@ protected IoWriteFuture doWritePacket(Buffer buffer) throws
IOException {
synchronized (encodeLock) {
Buffer packet = resolveOutputPacket(buffer);
IoSession networkSession = getIoSession();
-return networkSession.writePacket(packet);
+IoWriteFuture future = networkSession.writePacket(packet);
+buffer.rpos(buffer.wpos());
+return future;
Review comment:
You are right - took me a bit to ensure that this is the case...
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #123: SFTP transfer speeds, clean API
lgoldstein commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r412178637
##
File path:
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##
@@ -954,7 +954,9 @@ protected IoWriteFuture doWritePacket(Buffer buffer) throws
IOException {
synchronized (encodeLock) {
Buffer packet = resolveOutputPacket(buffer);
IoSession networkSession = getIoSession();
-return networkSession.writePacket(packet);
+IoWriteFuture future = networkSession.writePacket(packet);
+buffer.rpos(buffer.wpos());
+return future;
Review comment:
>> So while the packet is yet to be written, the buffer won't be used
anymore and is consumed at the time the buffer.rpos(buffer.wpos()) is called.
That was not what I meant - `networkSession#writePacket` might be
asynchronous - i.e., posts the buffer for writing and returns immediately - so
when it attempt to access `rpos` it might use the wrong one since we override
it.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #123: SFTP transfer speeds, clean API
lgoldstein commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r412176392
##
File path:
sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/SftpRemotePathChannel.java
##
@@ -524,13 +527,13 @@ protected void endBlocking(Object actionHint, boolean
completed)
* @param reqModesThe required modes - ignored if {@code null}/empty
* @throws IOException If channel not open or the required modes are not
satisfied
*/
-private void ensureOpen(Collection reqModes) throws
IOException {
+private void ensureOpen(Collection reqModes) throws IOException {
Review comment:
+1
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #123: SFTP transfer speeds, clean API
lgoldstein commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r412176062
##
File path:
sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTransferTest.java
##
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.client.subsystem.sftp;
+
+import java.io.BufferedWriter;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.Date;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.sshd.client.session.ClientSession;
+import org.apache.sshd.client.subsystem.sftp.fs.SftpFileSystem;
+import org.junit.Test;
+
+public class SftpTransferTest extends AbstractSftpClientTestSupport {
Review comment:
>> I'll add it, but not sure when it's actually useful...
My experience shows that it is possible for a unit test to fail due to some
unforeseen dependency with some side-effect of some other test. However, if the
order of the tests changes from one run to another (can happen if using a
different JDK version) then the failure may become irreproducible. Therefore,
the _surefire_ plugin is configured to run the classes in alphabetical order -
but within each class we want to make sure the individual tests are also run in
a predictable order. This way, if a unit test fails (for whatever reason - not
only because of interaction with another test) we can re-produce it with a high
probability.
Of course, the down side is that if there is some interaction we don't know
about we might not discover it until by some chance the lexicographical order
changes. This can be managed by having a **separate** build profile that
randomizes the methods, lists the chosen permutation and then runs the tests in
that particular order. This way, if there is some unknown interaction between
the tests we might detect it eventually and then reproduce it using the listed
permutation order.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #123: SFTP transfer speeds, clean API
lgoldstein commented on a change in pull request #123: URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411554606 ## File path: sshd-sftp/pom.xml ## @@ -85,6 +85,18 @@ jzlib test + +org.testcontainers +testcontainers +1.12.5 +test + + +org.testcontainers +toxiproxy +1.12.5 +test + Review comment: Even better... 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: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #123: SFTP transfer speeds, clean API
lgoldstein commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411387849
##
File path:
sshd-core/src/main/java/org/apache/sshd/common/channel/ChannelAsyncOutputStream.java
##
@@ -107,9 +108,39 @@ protected synchronized void doWriteIfPossible(boolean
resume) {
if (total > 0) {
Channel channel = getChannel();
Window remoteWindow = channel.getRemoteWindow();
-long length = Math.min(Math.min(remoteWindow.getSize(), total),
remoteWindow.getPacketSize());
-if (log.isTraceEnabled()) {
-log.trace("doWriteIfPossible({})[resume={}] attempting to
write {} out of {}", this, resume, length, total);
+long length;
+if (remoteWindow.getSize() < total && total <=
remoteWindow.getPacketSize()) {
+// do not chunk when the window is smaller than the packet size
+length = 0;
+// do a defensive copy in case the user reuses the buffer
+IoWriteFutureImpl f = new IoWriteFutureImpl(future.getId(),
new ByteArrayBuffer(buffer.getCompactData()));
+f.addListener(w -> future.setValue(w.getException() != null ?
w.getException() : w.isWritten()));
+pendingWrite.set(f);
+if (log.isTraceEnabled()) {
+log.trace("doWriteIfPossible({})[resume={}] waiting for
window space {}", this, resume,
+remoteWindow.getSize());
+}
+} else if (total > remoteWindow.getPacketSize()) {
+if (buffer.rpos() > 0) {
+// do a defensive copy in case the user reuses the buffer
Review comment:
This is a lot of code in a single method - can we somehow separate each
case to a (protected) method - easier to read, debug, maintain...
##
File path:
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##
@@ -954,7 +954,9 @@ protected IoWriteFuture doWritePacket(Buffer buffer) throws
IOException {
synchronized (encodeLock) {
Buffer packet = resolveOutputPacket(buffer);
IoSession networkSession = getIoSession();
-return networkSession.writePacket(packet);
+IoWriteFuture future = networkSession.writePacket(packet);
+buffer.rpos(buffer.wpos());
+return future;
Review comment:
Doesn't this change the `rpos` while the session is writing the buffer ?
Feels like this should be done only **after** packet has been written
##
File path: sshd-sftp/pom.xml
##
@@ -85,6 +85,18 @@
jzlib
test
+
+org.testcontainers
+testcontainers
+1.12.5
+test
+
+
+org.testcontainers
+toxiproxy
+1.12.5
+test
+
Review comment:
If these 2 dependencies are always expected to have the same version
let's use a property (e.g., `${testcontainers.version}` - this way if we
upgrade one we will not forget to update the other (D.R.Y. principle)
##
File path:
sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/SftpOutputStreamAsync.java
##
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.client.subsystem.sftp.impl;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Deque;
+import java.util.LinkedList;
+import java.util.Objects;
+
+import org.apache.sshd.client.subsystem.sftp.SftpClient;
+import org.apache.sshd.client.subsystem.sftp.SftpClient.CloseableHandle;
+import org.apache.sshd.client.subsystem.sftp.SftpClient.OpenMode;
+import org.apache.sshd.common.SshConstants;
+import org.apache.sshd.common.subsystem.sftp.SftpConstants;
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+import org.apache.sshd.common.util.io.OutputStreamWithChannel;
+
+/**
+ * Implements an output stream for a given remote file
+ *
+ * @author mailto:dev@mina.apache.org";>Apache MINA SSHD Projec
