hello Sai, I didn't have time to fully review the code but i made these nearly random changes in IODataConnectionFactory and DefaultDataConnectionConfiguration and the server is not getting blocked anymore...
Index: main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java =================================================================== --- main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (revision 927354) +++ main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (working copy) @@ -129,7 +129,8 @@ * port will be used. */ public synchronized int requestPassivePort() { - int dataPort = -1; + + int dataPort = -1; int loopTimes = 2; Thread currThread = Thread.currentThread(); @@ -164,8 +165,7 @@ */ public synchronized void releasePassivePort(final int port) { passivePorts.releasePort(port); - - notify(); + notifyAll(); } /** Index: main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java =================================================================== --- main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (revision 927354) +++ main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (working copy) @@ -44,446 +44,459 @@ * * We can get the FTP data connection using this class. It uses either PORT or * PASV command. - * + * * @author <a href="http://mina.apache.org">Apache MINA Project</a> */ public class IODataConnectionFactory implements ServerDataConnectionFactory { - private final Logger LOG = LoggerFactory - .getLogger(IODataConnectionFactory.class); + private final Logger LOG = LoggerFactory + .getLogger(IODataConnectionFactory.class); - private FtpServerContext serverContext; + private FtpServerContext serverContext; - private Socket dataSoc; + private Socket dataSoc; - ServerSocket servSoc; + ServerSocket servSoc; - InetAddress address; + InetAddress address; - int port = 0; + int port = 0; - long requestTime = 0L; + long requestTime = 0L; - boolean passive = false; + boolean passive = false; - boolean secure = false; + boolean secure = false; - private boolean isZip = false; + private boolean isZip = false; - InetAddress serverControlAddress; + InetAddress serverControlAddress; - FtpIoSession session; + FtpIoSession session; - public IODataConnectionFactory(final FtpServerContext serverContext, - final FtpIoSession session) { - this.session = session; - this.serverContext = serverContext; - if (session.getListener().getDataConnectionConfiguration() - .isImplicitSsl()) { - secure = true; - } - } + public IODataConnectionFactory(final FtpServerContext serverContext, + final FtpIoSession session) { + this.session = session; + this.serverContext = serverContext; + if (session.getListener().getDataConnectionConfiguration() + .isImplicitSsl()) { + secure = true; + } + } - /** - * Close data socket. - * This method must be idempotent as we might call it multiple times during disconnect. - */ - public synchronized void closeDataConnection() { + /** + * Close data socket. This method must be idempotent as we might call it + * multiple times during disconnect. + */ + public synchronized void closeDataConnection() { - // close client socket if any - if (dataSoc != null) { - try { - dataSoc.close(); - } catch (Exception ex) { - LOG.warn("FtpDataConnection.closeDataSocket()", ex); - } - dataSoc = null; - } + // close client socket if any + if (dataSoc != null) { + try { + dataSoc.close(); + } catch (Exception ex) { + LOG.warn("FtpDataConnection.closeDataSocket()", ex); + } + dataSoc = null; + } - // close server socket if any - if (servSoc != null) { - try { - servSoc.close(); - } catch (Exception ex) { - LOG.warn("FtpDataConnection.closeDataSocket()", ex); - } + // close server socket if any + if (servSoc != null) { + try { + servSoc.close(); + } catch (Exception ex) { + LOG.warn("FtpDataConnection.closeDataSocket()", ex); + } - if (session != null) { - DataConnectionConfiguration dcc = session.getListener() - .getDataConnectionConfiguration(); - if (dcc != null) { - dcc.releasePassivePort(port); - } - } + servSoc = null; + } - servSoc = null; - } + // release passive ports + if (session != null) { + DataConnectionConfiguration dcc = session.getListener() + .getDataConnectionConfiguration(); + if (dcc != null) { + dcc.releasePassivePort(port); + } + } + + // reset request time + requestTime = 0L; + } - // reset request time - requestTime = 0L; - } + /** + * Port command. + */ + public synchronized void initActiveDataConnection( + final InetSocketAddress address) { - /** - * Port command. - */ - public synchronized void initActiveDataConnection( - final InetSocketAddress address) { + // close old sockets if any + closeDataConnection(); - // close old sockets if any - closeDataConnection(); + // set variables + passive = false; + this.address = address.getAddress(); + port = address.getPort(); + requestTime = System.currentTimeMillis(); + } - // set variables - passive = false; - this.address = address.getAddress(); - port = address.getPort(); - requestTime = System.currentTimeMillis(); - } + private SslConfiguration getSslConfiguration() { + DataConnectionConfiguration dataCfg = session.getListener() + .getDataConnectionConfiguration(); - private SslConfiguration getSslConfiguration() { - DataConnectionConfiguration dataCfg = session.getListener() - .getDataConnectionConfiguration(); + SslConfiguration configuration = dataCfg.getSslConfiguration(); - SslConfiguration configuration = dataCfg.getSslConfiguration(); + // fall back if no configuration has been provided on the data + // connection config + if (configuration == null) { + configuration = session.getListener().getSslConfiguration(); + } - // fall back if no configuration has been provided on the data connection config - if (configuration == null) { - configuration = session.getListener().getSslConfiguration(); - } + return configuration; + } - return configuration; - } + /** + * Initiate a data connection in passive mode (server listening). + */ + public synchronized InetSocketAddress initPassiveDataConnection() + throws DataConnectionException { + LOG.debug("Initiating passive data connection"); + // close old sockets if any + closeDataConnection(); - /** - * Initiate a data connection in passive mode (server listening). - */ - public synchronized InetSocketAddress initPassiveDataConnection() - throws DataConnectionException { - LOG.debug("Initiating passive data connection"); - // close old sockets if any - closeDataConnection(); + // get the passive port + int passivePort = session.getListener() + .getDataConnectionConfiguration().requestPassivePort(); + if (passivePort == -1) { + servSoc = null; + throw new DataConnectionException( + "Cannot find an available passive port."); + } - // get the passive port - int passivePort = session.getListener() - .getDataConnectionConfiguration().requestPassivePort(); - if (passivePort == -1) { - servSoc = null; - throw new DataConnectionException( - "Cannot find an available passive port."); - } + // open passive server socket and get parameters + try { + DataConnectionConfiguration dataCfg = session.getListener() + .getDataConnectionConfiguration(); - // open passive server socket and get parameters - try { - DataConnectionConfiguration dataCfg = session.getListener() - .getDataConnectionConfiguration(); + String passiveAddress = dataCfg.getPassiveAddress(); - String passiveAddress = dataCfg.getPassiveAddress(); + if (passiveAddress == null) { + address = serverControlAddress; + } else { + address = resolveAddress(dataCfg.getPassiveAddress()); + } - if (passiveAddress == null) { - address = serverControlAddress; - } else { - address = resolveAddress(dataCfg.getPassiveAddress()); - } + if (secure) { + LOG + .debug( + "Opening SSL passive data connection on address \"{}\" and port {}", + address, passivePort); + SslConfiguration ssl = getSslConfiguration(); + if (ssl == null) { + throw new DataConnectionException( + "Data connection SSL required but not configured."); + } - if (secure) { - LOG - .debug( - "Opening SSL passive data connection on address \"{}\" and port {}", - address, passivePort); - SslConfiguration ssl = getSslConfiguration(); - if (ssl == null) { - throw new DataConnectionException( - "Data connection SSL required but not configured."); - } + // this method does not actually create the SSL socket, due to a + // JVM bug + // (https://issues.apache.org/jira/browse/FTPSERVER-241). + // Instead, it creates a regular + // ServerSocket that will be wrapped as a SSL socket in + // createDataSocket() + servSoc = new ServerSocket(passivePort, 0, address); + LOG + .debug( + "SSL Passive data connection created on address \"{}\" and port {}", + address, passivePort); + } else { + LOG + .debug( + "Opening passive data connection on address \"{}\" and port {}", + address, passivePort); + servSoc = new ServerSocket(passivePort, 0, address); + LOG + .debug( + "Passive data connection created on address \"{}\" and port {}", + address, passivePort); + } + port = servSoc.getLocalPort(); + servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000); - // this method does not actually create the SSL socket, due to a JVM bug - // (https://issues.apache.org/jira/browse/FTPSERVER-241). - // Instead, it creates a regular - // ServerSocket that will be wrapped as a SSL socket in createDataSocket() - servSoc = new ServerSocket(passivePort, 0, address); - LOG - .debug( - "SSL Passive data connection created on address \"{}\" and port {}", - address, passivePort); - } else { - LOG - .debug( - "Opening passive data connection on address \"{}\" and port {}", - address, passivePort); - servSoc = new ServerSocket(passivePort, 0, address); - LOG - .debug( - "Passive data connection created on address \"{}\" and port {}", - address, passivePort); - } - port = servSoc.getLocalPort(); - servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000); + // set different state variables + passive = true; + requestTime = System.currentTimeMillis(); - // set different state variables - passive = true; - requestTime = System.currentTimeMillis(); + return new InetSocketAddress(address, port); + } catch (Exception ex) { + servSoc = null; + closeDataConnection(); + throw new DataConnectionException( + "Failed to initate passive data connection: " + + ex.getMessage(), ex); + } + } - return new InetSocketAddress(address, port); - } catch (Exception ex) { - servSoc = null; - closeDataConnection(); - throw new DataConnectionException( - "Failed to initate passive data connection: " - + ex.getMessage(), ex); - } - } + /* + * (non-Javadoc) + * + * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress() + */ + public InetAddress getInetAddress() { + return address; + } - /* - * (non-Javadoc) - * - * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress() - */ - public InetAddress getInetAddress() { - return address; - } + /* + * (non-Javadoc) + * + * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort() + */ + public int getPort() { + return port; + } - /* - * (non-Javadoc) - * - * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort() - */ - public int getPort() { - return port; - } + /* + * (non-Javadoc) + * + * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection() + */ + public DataConnection openConnection() throws Exception { + return new IODataConnection(createDataSocket(), session, this); + } - /* - * (non-Javadoc) - * - * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection() - */ - public DataConnection openConnection() throws Exception { - return new IODataConnection(createDataSocket(), session, this); - } + /** + * Get the data socket. In case of error returns null. + */ + private synchronized Socket createDataSocket() throws Exception { - /** - * Get the data socket. In case of error returns null. - */ - private synchronized Socket createDataSocket() throws Exception { + // get socket depending on the selection + dataSoc = null; + DataConnectionConfiguration dataConfig = session.getListener() + .getDataConnectionConfiguration(); + try { + if (!passive) { + if (secure) { + LOG.debug("Opening secure active data connection"); + SslConfiguration ssl = getSslConfiguration(); + if (ssl == null) { + throw new FtpException( + "Data connection SSL not configured"); + } - // get socket depending on the selection - dataSoc = null; - DataConnectionConfiguration dataConfig = session.getListener() - .getDataConnectionConfiguration(); - try { - if (!passive) { - if (secure) { - LOG.debug("Opening secure active data connection"); - SslConfiguration ssl = getSslConfiguration(); - if (ssl == null) { - throw new FtpException( - "Data connection SSL not configured"); - } + // get socket factory + SSLSocketFactory socFactory = ssl.getSocketFactory(); - // get socket factory - SSLSocketFactory socFactory = ssl.getSocketFactory(); + // create socket + SSLSocket ssoc = (SSLSocket) socFactory.createSocket(); + ssoc.setUseClientMode(false); - // create socket - SSLSocket ssoc = (SSLSocket) socFactory.createSocket(); - ssoc.setUseClientMode(false); + // initialize socket + if (ssl.getEnabledCipherSuites() != null) { + ssoc.setEnabledCipherSuites(ssl + .getEnabledCipherSuites()); + } + dataSoc = ssoc; + } else { + LOG.debug("Opening active data connection"); + dataSoc = new Socket(); + } - // initialize socket - if (ssl.getEnabledCipherSuites() != null) { - ssoc.setEnabledCipherSuites(ssl.getEnabledCipherSuites()); - } - dataSoc = ssoc; - } else { - LOG.debug("Opening active data connection"); - dataSoc = new Socket(); - } + dataSoc.setReuseAddress(true); - dataSoc.setReuseAddress(true); + InetAddress localAddr = resolveAddress(dataConfig + .getActiveLocalAddress()); - InetAddress localAddr = resolveAddress(dataConfig - .getActiveLocalAddress()); + // if no local address has been configured, make sure we use the + // same as the client connects from + if (localAddr == null) { + localAddr = ((InetSocketAddress) session.getLocalAddress()) + .getAddress(); + } - // if no local address has been configured, make sure we use the same as the client connects from - if(localAddr == null) { - localAddr = ((InetSocketAddress)session.getLocalAddress()).getAddress(); - } + SocketAddress localSocketAddress = new InetSocketAddress( + localAddr, dataConfig.getActiveLocalPort()); - SocketAddress localSocketAddress = new InetSocketAddress(localAddr, dataConfig.getActiveLocalPort()); - - LOG.debug("Binding active data connection to {}", localSocketAddress); - dataSoc.bind(localSocketAddress); + LOG.debug("Binding active data connection to {}", + localSocketAddress); + dataSoc.bind(localSocketAddress); - dataSoc.connect(new InetSocketAddress(address, port)); - } else { + dataSoc.connect(new InetSocketAddress(address, port)); + } else { - if (secure) { - LOG.debug("Opening secure passive data connection"); - // this is where we wrap the unsecured socket as a SSLSocket. This is - // due to the JVM bug described in FTPSERVER-241. + if (secure) { + LOG.debug("Opening secure passive data connection"); + // this is where we wrap the unsecured socket as a + // SSLSocket. This is + // due to the JVM bug described in FTPSERVER-241. - // get server socket factory - SslConfiguration ssl = getSslConfiguration(); - - // we've already checked this, but let's do it again - if (ssl == null) { - throw new FtpException( - "Data connection SSL not configured"); - } + // get server socket factory + SslConfiguration ssl = getSslConfiguration(); - SSLSocketFactory ssocketFactory = ssl.getSocketFactory(); + // we've already checked this, but let's do it again + if (ssl == null) { + throw new FtpException( + "Data connection SSL not configured"); + } - Socket serverSocket = servSoc.accept(); + SSLSocketFactory ssocketFactory = ssl.getSocketFactory(); - SSLSocket sslSocket = (SSLSocket) ssocketFactory - .createSocket(serverSocket, serverSocket - .getInetAddress().getHostAddress(), - serverSocket.getPort(), true); - sslSocket.setUseClientMode(false); + Socket serverSocket = servSoc.accept(); - // initialize server socket - if (ssl.getClientAuth() == ClientAuth.NEED) { - sslSocket.setNeedClientAuth(true); - } else if (ssl.getClientAuth() == ClientAuth.WANT) { - sslSocket.setWantClientAuth(true); - } + SSLSocket sslSocket = (SSLSocket) ssocketFactory + .createSocket(serverSocket, serverSocket + .getInetAddress().getHostAddress(), + serverSocket.getPort(), true); + sslSocket.setUseClientMode(false); - if (ssl.getEnabledCipherSuites() != null) { - sslSocket.setEnabledCipherSuites(ssl - .getEnabledCipherSuites()); - } + // initialize server socket + if (ssl.getClientAuth() == ClientAuth.NEED) { + sslSocket.setNeedClientAuth(true); + } else if (ssl.getClientAuth() == ClientAuth.WANT) { + sslSocket.setWantClientAuth(true); + } - dataSoc = sslSocket; - } else { - LOG.debug("Opening passive data connection"); + if (ssl.getEnabledCipherSuites() != null) { + sslSocket.setEnabledCipherSuites(ssl + .getEnabledCipherSuites()); + } - dataSoc = servSoc.accept(); - } - - if (dataConfig.isPassiveIpCheck()) { + dataSoc = sslSocket; + } else { + LOG.debug("Opening passive data connection"); + + dataSoc = servSoc.accept(); + } + + if (dataConfig.isPassiveIpCheck()) { // Let's make sure we got the connection from the same // client that we are expecting - InetAddress remoteAddress = ((InetSocketAddress) session.getRemoteAddress()).getAddress(); + InetAddress remoteAddress = ((InetSocketAddress) session + .getRemoteAddress()).getAddress(); InetAddress dataSocketAddress = dataSoc.getInetAddress(); if (!dataSocketAddress.equals(remoteAddress)) { - LOG.warn("Passive IP Check failed. Closing data connection from " - + dataSocketAddress - + " as it does not match the expected address " - + remoteAddress); + LOG + .warn("Passive IP Check failed. Closing data connection from " + + dataSocketAddress + + " as it does not match the expected address " + + remoteAddress); closeDataConnection(); return null; } } - - DataConnectionConfiguration dataCfg = session.getListener() - .getDataConnectionConfiguration(); - - dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000); - LOG.debug("Passive data connection opened"); - } - } catch (Exception ex) { - closeDataConnection(); - LOG.warn("FtpDataConnection.getDataSocket()", ex); - throw ex; - } - dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000); - // Make sure we initiate the SSL handshake, or we'll - // get an error if we turn out not to send any data - // e.g. during the listing of an empty directory - if (dataSoc instanceof SSLSocket) { - ((SSLSocket) dataSoc).startHandshake(); - } + DataConnectionConfiguration dataCfg = session.getListener() + .getDataConnectionConfiguration(); - return dataSoc; - } + dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000); + LOG.debug("Passive data connection opened"); + } + } catch (Exception ex) { + closeDataConnection(); + LOG.warn("FtpDataConnection.getDataSocket()", ex); + throw ex; + } + dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000); - /* - * (non-Javadoc) - * Returns an InetAddress object from a hostname or IP address. - */ - private InetAddress resolveAddress(String host) - throws DataConnectionException { - if (host == null) { - return null; - } else { - try { - return InetAddress.getByName(host); - } catch (UnknownHostException ex) { - throw new DataConnectionException("Failed to resolve address", ex); - } - } - } + // Make sure we initiate the SSL handshake, or we'll + // get an error if we turn out not to send any data + // e.g. during the listing of an empty directory + if (dataSoc instanceof SSLSocket) { + ((SSLSocket) dataSoc).startHandshake(); + } - /* - * (non-Javadoc) - * - * @see org.apache.ftpserver.DataConnectionFactory#isSecure() - */ - public boolean isSecure() { - return secure; - } + return dataSoc; + } - /** - * Set the security protocol. - */ - public void setSecure(final boolean secure) { - this.secure = secure; - } + /* + * (non-Javadoc) Returns an InetAddress object from a hostname or IP + * address. + */ + private InetAddress resolveAddress(String host) + throws DataConnectionException { + if (host == null) { + return null; + } else { + try { + return InetAddress.getByName(host); + } catch (UnknownHostException ex) { + throw new DataConnectionException("Failed to resolve address", + ex); + } + } + } - /* - * (non-Javadoc) - * - * @see org.apache.ftpserver.DataConnectionFactory#isZipMode() - */ - public boolean isZipMode() { - return isZip; - } + /* + * (non-Javadoc) + * + * @see org.apache.ftpserver.DataConnectionFactory#isSecure() + */ + public boolean isSecure() { + return secure; + } - /** - * Set zip mode. - */ - public void setZipMode(final boolean zip) { - isZip = zip; - } + /** + * Set the security protocol. + */ + public void setSecure(final boolean secure) { + this.secure = secure; + } - /** - * Check the data connection idle status. - */ - public synchronized boolean isTimeout(final long currTime) { + /* + * (non-Javadoc) + * + * @see org.apache.ftpserver.DataConnectionFactory#isZipMode() + */ + public boolean isZipMode() { + return isZip; + } - // data connection not requested - not a timeout - if (requestTime == 0L) { - return false; - } + /** + * Set zip mode. + */ + public void setZipMode(final boolean zip) { + isZip = zip; + } - // data connection active - not a timeout - if (dataSoc != null) { - return false; - } + /** + * Check the data connection idle status. + */ + public synchronized boolean isTimeout(final long currTime) { - // no idle time limit - not a timeout - int maxIdleTime = session.getListener() - .getDataConnectionConfiguration().getIdleTime() * 1000; - if (maxIdleTime == 0) { - return false; - } + // data connection not requested - not a timeout + if (requestTime == 0L) { + return false; + } - // idle time is within limit - not a timeout - if ((currTime - requestTime) < maxIdleTime) { - return false; - } + // data connection active - not a timeout + if (dataSoc != null) { + return false; + } - return true; - } + // no idle time limit - not a timeout + int maxIdleTime = session.getListener() + .getDataConnectionConfiguration().getIdleTime() * 1000; + if (maxIdleTime == 0) { + return false; + } - /** - * Dispose data connection - close all the sockets. - */ - public void dispose() { - closeDataConnection(); - } + // idle time is within limit - not a timeout + if ((currTime - requestTime) < maxIdleTime) { + return false; + } - /** - * Sets the server's control address. - */ - public void setServerControlAddress(final InetAddress serverControlAddress) { - this.serverControlAddress = serverControlAddress; - } + return true; + } + + /** + * Dispose data connection - close all the sockets. + */ + public void dispose() { + closeDataConnection(); + } + + /** + * Sets the server's control address. + */ + public void setServerControlAddress(final InetAddress serverControlAddress) { + this.serverControlAddress = serverControlAddress; + } } 2010/3/25 Sai Pullabhotla <sai.pullabho...@jmethods.com>: > Just so we are all on the same page - > > Do you think the issue is with the LIST command, or the code that > creates passive data connection? Could you briefly explain some of the > issues that made you think it should be rewritten, so everyone gets a > chance to evaluate? > > Thanks. > > Regards, > Sai Pullabhotla > > > > > > On Thu, Mar 25, 2010 at 5:56 AM, David Latorre <dvl...@gmail.com> wrote: >> 2010/3/24 Niklas Gustavsson <nik...@protocol7.com>: >>> On Wed, Mar 24, 2010 at 6:03 PM, Fred Moore <fred.moor...@gmail.com> wrote: >>>> we found an issue related to requestPassivePort() which may lead to an >>>> unresponsive V1.0.4 FTP (or FTP/S) Server, this issue can be reproduced. >>>> >>>> http://issues.apache.org/jira/browse/FTPSERVER-359 contains full >>>> description >>>> of the symptoms and a minimalist java client and server to reproduce it. >>> >>> I haven't yet looked closer at the code you attached. But, I have seen >>> similar behavior myself during performance testing FtpServer. In that >>> case, I had a very similar behavior at around 20 threads. However, the >>> reason for the problem in that test was that the test case uses up >>> file handles (for the sockets) so fast that they will run out. Since >>> sockets hang around for some time also after closing, they were not >>> freed quickly enough and thus FtpServer could not open new ones. Could >>> you please verify that this is not the case here? You could look at >>> netstat output and look into increasing the allowed number of file >>> handles our the timeout time for sockets in your OS. >> >> >> Actually it is quite easy to reproduce this error (I just wrote a >> client test case with throws >20~30 threads that list a directory in >> the server ) and it's not file handle related: >> we have several bugs in our code that cause this behaviour , i >> think we hould rewrite all the request/release passive port mechanism >> as there are several issues with it. >> >> >> >> >> >> >> >>> /niklas >>> >> >