You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mina.apache.org by lg...@apache.org on 2019/01/29 05:55:17 UTC
[mina-sshd] 03/05: [SSHD-881] Use an internal chunking mechanism
for AbstractSftpClient#write implementation
This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 34a65aa6a11f95e71ac6355939690a7d391ca7bd
Author: Lyor Goldstein <lg...@apache.org>
AuthorDate: Sun Jan 13 13:35:21 2019 +0200
[SSHD-881] Use an internal chunking mechanism for AbstractSftpClient#write implementation
---
.../subsystem/sftp/impl/AbstractSftpClient.java | 46 +++++++++++++++++-----
.../sshd/client/subsystem/sftp/SftpTest.java | 25 +++++++-----
2 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/AbstractSftpClient.java b/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/AbstractSftpClient.java
index e10b466..73e3969 100644
--- a/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/AbstractSftpClient.java
+++ b/sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/AbstractSftpClient.java
@@ -38,6 +38,7 @@ import org.apache.sshd.client.subsystem.sftp.SftpClient;
import org.apache.sshd.client.subsystem.sftp.extensions.BuiltinSftpClientExtensions;
import org.apache.sshd.client.subsystem.sftp.extensions.SftpClientExtension;
import org.apache.sshd.client.subsystem.sftp.extensions.SftpClientExtensionFactory;
+import org.apache.sshd.common.PropertyResolverUtils;
import org.apache.sshd.common.SshConstants;
import org.apache.sshd.common.SshException;
import org.apache.sshd.common.channel.Channel;
@@ -50,11 +51,21 @@ import org.apache.sshd.common.util.ValidateUtils;
import org.apache.sshd.common.util.buffer.Buffer;
import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
-
/**
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
public abstract class AbstractSftpClient extends AbstractSubsystemClient implements SftpClient, RawSftpClient {
+ /**
+ * Property used to avoid large buffers when {@link #write(Handle, long, byte[], int, int)} is invoked
+ * with a large buffer size.
+ */
+ public static final String WRITE_CHUNK_SIZE = "sftp-client-write-chunk-size";
+
+ /**
+ * Default value for {@value #WRITE_CHUNK_SIZE}
+ */
+ public static final int DEFAULT_WRITE_CHUNK_SIZE = SshConstants.SSH_REQUIRED_PAYLOAD_PACKET_LENGTH_SUPPORT - Long.SIZE;
+
private final Attributes fileOpenAttributes = new Attributes();
private final AtomicReference<Map<String, Object>> parsedExtensionsHolder = new AtomicReference<>(null);
@@ -870,17 +881,32 @@ public abstract class AbstractSftpClient extends AbstractSubsystemClient impleme
throw new IOException("write(" + handle + "/" + fileOffset + ")[" + srcOffset + "/" + len + "] client is closed");
}
- if (log.isTraceEnabled()) {
- log.trace("write({}) handle={}, file-offset={}, buf-offset={}, len={}",
- getClientChannel(), handle, fileOffset, srcOffset, len);
- }
+ boolean traceEnabled = log.isTraceEnabled();
+ Channel clientChannel = getClientChannel();
+ int chunkSize = PropertyResolverUtils.getIntProperty(clientChannel, WRITE_CHUNK_SIZE, DEFAULT_WRITE_CHUNK_SIZE);
+ ValidateUtils.checkState(chunkSize > ByteArrayBuffer.DEFAULT_SIZE, "Write chunk size too small: %d", chunkSize);
byte[] id = Objects.requireNonNull(handle, "No handle").getIdentifier();
- Buffer buffer = new ByteArrayBuffer(id.length + len + Long.SIZE /* some extra fields */, false);
- buffer.putBytes(id);
- buffer.putLong(fileOffset);
- buffer.putBytes(src, srcOffset, len);
- checkCommandStatus(SftpConstants.SSH_FXP_WRITE, buffer);
+ // NOTE: we don't want to filter out zero-length write requests
+ int remLen = len;
+ do {
+ int writeSize = Math.min(remLen, chunkSize);
+ Buffer buffer = new ByteArrayBuffer(id.length + writeSize + Long.SIZE /* some extra fields */, false);
+ buffer.putBytes(id);
+ buffer.putLong(fileOffset);
+ buffer.putBytes(src, srcOffset, writeSize);
+
+ if (traceEnabled) {
+ log.trace("write({}) handle={}, file-offset={}, buf-offset={}, writeSize={}, remLen={}",
+ clientChannel, handle, fileOffset, srcOffset, writeSize, remLen - writeSize);
+ }
+
+ checkCommandStatus(SftpConstants.SSH_FXP_WRITE, buffer);
+
+ fileOffset += writeSize;
+ srcOffset += writeSize;
+ remLen -= writeSize;
+ } while (remLen > 0);
}
@Override
diff --git a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java
index b58cfb2..5a07733 100644
--- a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java
+++ b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTest.java
@@ -57,6 +57,7 @@ import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
+import org.apache.sshd.client.channel.ClientChannel;
import org.apache.sshd.client.session.ClientSession;
import org.apache.sshd.client.subsystem.sftp.SftpClient.Attributes;
import org.apache.sshd.client.subsystem.sftp.SftpClient.CloseableHandle;
@@ -64,11 +65,13 @@ import org.apache.sshd.client.subsystem.sftp.SftpClient.DirEntry;
import org.apache.sshd.client.subsystem.sftp.SftpClient.OpenMode;
import org.apache.sshd.client.subsystem.sftp.extensions.BuiltinSftpClientExtensions;
import org.apache.sshd.client.subsystem.sftp.extensions.SftpClientExtension;
+import org.apache.sshd.client.subsystem.sftp.impl.AbstractSftpClient;
import org.apache.sshd.common.Factory;
import org.apache.sshd.common.FactoryManager;
import org.apache.sshd.common.NamedFactory;
import org.apache.sshd.common.OptionalFeature;
import org.apache.sshd.common.PropertyResolverUtils;
+import org.apache.sshd.common.SshConstants;
import org.apache.sshd.common.channel.WindowClosedException;
import org.apache.sshd.common.channel.exception.SshChannelClosedException;
import org.apache.sshd.common.file.virtualfs.VirtualFileSystemFactory;
@@ -85,7 +88,6 @@ import org.apache.sshd.common.subsystem.sftp.extensions.openssh.AbstractOpenSSHE
import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.common.util.OsUtils;
import org.apache.sshd.common.util.buffer.BufferUtils;
-import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
import org.apache.sshd.common.util.io.IoUtils;
import org.apache.sshd.server.command.Command;
import org.apache.sshd.server.session.ServerSession;
@@ -836,14 +838,19 @@ public class SftpTest extends AbstractSftpClientTestSupport {
uploadAndVerifyFile(sftp, clientFolder, dir, 0, "emptyFile.txt");
uploadAndVerifyFile(sftp, clientFolder, dir, 1000, "smallFile.txt");
- uploadAndVerifyFile(sftp, clientFolder, dir, ByteArrayBuffer.MAX_LEN - 1, "bufferMaxLenMinusOneFile.txt");
- uploadAndVerifyFile(sftp, clientFolder, dir, ByteArrayBuffer.MAX_LEN, "bufferMaxLenFile.txt");
- // were chunking not implemented, these would fail. these sizes should invoke our internal chunking mechanism
- uploadAndVerifyFile(sftp, clientFolder, dir, ByteArrayBuffer.MAX_LEN + 1, "bufferMaxLenPlusOneFile.txt");
- uploadAndVerifyFile(sftp, clientFolder, dir, (int) (1.5 * ByteArrayBuffer.MAX_LEN), "1point5BufferMaxLenFile.txt");
- uploadAndVerifyFile(sftp, clientFolder, dir, (2 * ByteArrayBuffer.MAX_LEN) - 1, "2TimesBufferMaxLenMinusOneFile.txt");
- uploadAndVerifyFile(sftp, clientFolder, dir, 2 * ByteArrayBuffer.MAX_LEN, "2TimesBufferMaxLenFile.txt");
- uploadAndVerifyFile(sftp, clientFolder, dir, (2 * ByteArrayBuffer.MAX_LEN) + 1, "2TimesBufferMaxLenPlusOneFile.txt");
+
+ // Make sure sizes should invoke our internal chunking mechanism
+ ClientChannel clientChannel = sftp.getClientChannel();
+ PropertyResolverUtils.updateProperty(clientChannel, AbstractSftpClient.WRITE_CHUNK_SIZE,
+ Math.min(SftpClient.DEFAULT_WRITE_BUFFER_SIZE, AbstractSftpClient.DEFAULT_WRITE_CHUNK_SIZE) - Byte.MAX_VALUE);
+
+ uploadAndVerifyFile(sftp, clientFolder, dir, SshConstants.SSH_REQUIRED_TOTAL_PACKET_LENGTH_SUPPORT - 1, "bufferMaxLenMinusOneFile.txt");
+ uploadAndVerifyFile(sftp, clientFolder, dir, SshConstants.SSH_REQUIRED_TOTAL_PACKET_LENGTH_SUPPORT, "bufferMaxLenFile.txt");
+ uploadAndVerifyFile(sftp, clientFolder, dir, SshConstants.SSH_REQUIRED_TOTAL_PACKET_LENGTH_SUPPORT + 1, "bufferMaxLenPlusOneFile.txt");
+ uploadAndVerifyFile(sftp, clientFolder, dir, (int) (1.5 * SshConstants.SSH_REQUIRED_TOTAL_PACKET_LENGTH_SUPPORT), "1point5BufferMaxLenFile.txt");
+ uploadAndVerifyFile(sftp, clientFolder, dir, (2 * SshConstants.SSH_REQUIRED_TOTAL_PACKET_LENGTH_SUPPORT) - 1, "2TimesBufferMaxLenMinusOneFile.txt");
+ uploadAndVerifyFile(sftp, clientFolder, dir, 2 * SshConstants.SSH_REQUIRED_TOTAL_PACKET_LENGTH_SUPPORT, "2TimesBufferMaxLenFile.txt");
+ uploadAndVerifyFile(sftp, clientFolder, dir, (2 * SshConstants.SSH_REQUIRED_TOTAL_PACKET_LENGTH_SUPPORT) + 1, "2TimesBufferMaxLenPlusOneFile.txt");
uploadAndVerifyFile(sftp, clientFolder, dir, 200000, "largerFile.txt");
// test erroneous calls that check for negative values