You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by gg...@apache.org on 2023/02/04 14:25:31 UTC

[commons-vfs] branch master updated: VFS-832: Sftp channel not put back in doGetInputStream (#370)

This is an automated email from the ASF dual-hosted git repository.

ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-vfs.git


The following commit(s) were added to refs/heads/master by this push:
     new 58fee7a0 VFS-832: Sftp channel not put back in doGetInputStream (#370)
58fee7a0 is described below

commit 58fee7a0472d6ae382c471f82c7fa79d89752a12
Author: Wangerry <wa...@live.cn>
AuthorDate: Sat Feb 4 22:25:27 2023 +0800

    VFS-832: Sftp channel not put back in doGetInputStream (#370)
---
 .../commons/vfs2/provider/sftp/SftpFileObject.java |   4 +
 .../sftp/AbstractSftpProviderTestCase.java         |  12 ++-
 .../vfs2/provider/sftp/SftpPutChannelTestCase.java | 112 +++++++++++++++++++++
 3 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileObject.java b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileObject.java
index 1d1236ff..25e546b9 100644
--- a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileObject.java
+++ b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileObject.java
@@ -176,10 +176,14 @@ public class SftpFileObject extends AbstractFileObject<SftpFileSystem> {
                 // VFS-210: sftp allows to gather an input stream even from a directory and will
                 // fail on first read. So we need to check the type anyway
                 if (!getType().hasContent()) {
+                    // VFS-832: Sftp channel should put back when throw an exception
+                    getAbstractFileSystem().putChannel(channel);
                     throw new FileSystemException("vfs.provider/read-not-file.error", getName());
                 }
                 inputStream = channel.get(relPath);
             } catch (final SftpException e) {
+                // VFS-832: Sftp channel should put back when catch an exception
+                getAbstractFileSystem().putChannel(channel);
                 if (e.id == ChannelSftp.SSH_FX_NO_SUCH_FILE) {
                     throw new FileNotFoundException(getName());
                 }
diff --git a/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/sftp/AbstractSftpProviderTestCase.java b/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/sftp/AbstractSftpProviderTestCase.java
index 36c2612c..e549005f 100644
--- a/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/sftp/AbstractSftpProviderTestCase.java
+++ b/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/sftp/AbstractSftpProviderTestCase.java
@@ -64,6 +64,7 @@ import org.apache.sshd.server.filesystem.NativeSshFile;
 import org.apache.sshd.server.keyprovider.PEMGeneratorHostKeyProvider;
 import org.apache.sshd.server.keyprovider.SimpleGeneratorHostKeyProvider;
 import org.apache.sshd.server.session.ServerSession;
+import org.apache.sshd.server.session.SessionFactory;
 import org.apache.sshd.server.sftp.SftpSubsystem;
 
 import com.jcraft.jsch.SftpATTRS;
@@ -224,16 +225,18 @@ abstract class AbstractSftpProviderTestCase extends AbstractProviderTestConfig {
 
     static class SftpProviderTestSuite extends ProviderTestSuite {
         private final boolean isExecChannelClosed;
+        private final SessionFactory sessionFactory;
 
         public SftpProviderTestSuite(final AbstractSftpProviderTestCase providerConfig) throws Exception {
             super(providerConfig);
             this.isExecChannelClosed = providerConfig.isExecChannelClosed();
+            this.sessionFactory = providerConfig.sessionFactory();
         }
 
         @Override
         protected void setUp() throws Exception {
             if (getSystemTestUriOverride() == null) {
-                setUpClass(isExecChannelClosed);
+                setUpClass(isExecChannelClosed, sessionFactory);
             }
             super.setUp();
         }
@@ -482,13 +485,14 @@ abstract class AbstractSftpProviderTestCase extends AbstractProviderTestConfig {
      * @throws FtpException
      * @throws IOException
      */
-    private static void setUpClass(final boolean isExecChannelClosed) throws IOException {
+    private static void setUpClass(final boolean isExecChannelClosed, SessionFactory sessionFactory) throws IOException {
         if (Server != null) {
             return;
         }
         // System.setProperty("vfs.sftp.sshdir", getTestDirectory() + "/../vfs.sftp.sshdir");
         final Path tmpDir = Paths.get(System.getProperty("java.io.tmpdir"));
         Server = SshServer.setUpDefaultServer();
+        Server.setSessionFactory(sessionFactory);
         Server.setPort(0);
         if (SecurityUtils.isBouncyCastleRegistered()) {
             // A temporary file will hold the key
@@ -592,6 +596,10 @@ abstract class AbstractSftpProviderTestCase extends AbstractProviderTestConfig {
 
     protected abstract boolean isExecChannelClosed();
 
+    protected SessionFactory sessionFactory() {
+        return null;
+    }
+
     /**
      * Prepares the file system manager.
      */
diff --git a/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/sftp/SftpPutChannelTestCase.java b/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/sftp/SftpPutChannelTestCase.java
new file mode 100644
index 00000000..c63bb144
--- /dev/null
+++ b/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/sftp/SftpPutChannelTestCase.java
@@ -0,0 +1,112 @@
+/*
+ * 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.commons.vfs2.provider.sftp;
+
+import org.apache.commons.vfs2.Capability;
+import org.apache.commons.vfs2.FileObject;
+import org.apache.commons.vfs2.FileSystemException;
+import org.apache.mina.core.session.IoSession;
+import org.apache.sshd.common.FactoryManager;
+import org.apache.sshd.common.session.AbstractSession;
+import org.apache.sshd.server.session.ServerSession;
+import org.apache.sshd.server.session.SessionFactory;
+import org.junit.Test;
+import org.junit.jupiter.api.Assertions;
+
+import java.io.InputStream;
+
+/**
+ * Test SftpFileObject.doGetInputStream return the channel to pool when throw an exception.
+ */
+public class SftpPutChannelTestCase extends AbstractSftpProviderTestCase {
+
+    /**
+     * to expose the channels count
+     */
+    private static class CustomServerSession extends ServerSession {
+        public CustomServerSession(FactoryManager server, IoSession ioSession) throws Exception {
+            super(server, ioSession);
+        }
+
+        public int getChannelsCount() {
+            return channels.size();
+        }
+    }
+
+    private static class CustomSessionFactory extends SessionFactory {
+        @Override
+        protected AbstractSession doCreateSession(IoSession ioSession) throws Exception {
+            return new CustomServerSession(server, ioSession);
+        }
+    }
+
+    private static final Integer MAX_CHANNELS = 10;
+
+    /**
+     * Creates the test suite for the sftp file system.
+     */
+    public static junit.framework.Test suite() throws Exception {
+        final SftpProviderTestSuite suite = new SftpProviderTestSuite(new SftpPutChannelTestCase()) {
+            @Override
+            protected void addBaseTests() throws Exception {
+                // Just tries to read
+                addTests(SftpPutChannelTestCase.class);
+            }
+        };
+        return suite;
+    }
+
+    @Override
+    protected SessionFactory sessionFactory() {
+        return new CustomSessionFactory();
+    }
+
+    /**
+     * Returns the capabilities required by the tests of this test case.
+     */
+    @Override
+    protected Capability[] getRequiredCapabilities() {
+        return new Capability[]{Capability.CREATE, Capability.DELETE, Capability.GET_TYPE, Capability.LIST_CHILDREN,
+            Capability.READ_CONTENT, Capability.WRITE_CONTENT};
+    }
+
+    @Override
+    protected boolean isExecChannelClosed() {
+        return false;
+    }
+
+    /**
+     * Test SftpFileObject.doGetInputStream return the channel to pool, when there is an exception
+     */
+    @Test
+    public void testDoGetInputStream() throws Exception {
+        final FileObject readFolder = getReadFolder();
+        // try MAX_CHANNELS * 2 times, then check channels count less than MAX_CHANNELS
+        // ( actually must <= 2, but less than MAX_CHANNELS is enough
+        for (int i = 0; i < MAX_CHANNELS * 2; i++) {
+            try {
+                try (InputStream ignored = readFolder.resolveFile("not-exists.txt").getContent().getInputStream()) {
+                    Assertions.fail("file should not be exists");
+                }
+            } catch (FileSystemException e) {
+                int channelsCount = ((CustomServerSession) Server.getActiveSessions().get(0)).getChannelsCount();
+                Assertions.assertTrue(channelsCount < MAX_CHANNELS, "channels count expected less than " + MAX_CHANNELS);
+            }
+        }
+    }
+
+}