Commons Net
  1. Commons Net
  2. NET-465

FTPClient setSendBufferSize and setReceiveBufferSize on data socket

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.1
    • Fix Version/s: 3.3
    • Component/s: FTP
    • Labels:
      None
    • Environment:

      All

      Description

      When sending large files the need to set the send and receive buffer sizes on the data socket is much more important than the command socket. Please either give 2 more setters or make the setters set the data socket and leave the command socket alone.

      1. ftp-bufsize.diff
        6 kB
        Bogdan Drozdowski

        Issue Links

          Activity

          Hide
          Bogdan Drozdowski added a comment -

          The buffer size already was for the data connections, but it was set only on the buffered input and output streams, not on the sockets. The attached file, ftp-bufsize.diff, fixes this. Now if the buffer size is greater than zero, it is used both on the streams and the sockets. If the buffer size is set to 0 or less, the defaults are used (nothing is set on the sockets and the buffered streams use the default size).

          Show
          Bogdan Drozdowski added a comment - The buffer size already was for the data connections, but it was set only on the buffered input and output streams, not on the sockets. The attached file, ftp-bufsize.diff, fixes this. Now if the buffer size is greater than zero, it is used both on the streams and the sockets. If the buffer size is set to 0 or less, the defaults are used (nothing is set on the sockets and the buffered streams use the default size).
          Hide
          Bogdan Drozdowski added a comment -

          I forgot about one possibility - you can create and install your own SocketFactory that creates Sockets with your own buffer size. Look at the DefaultSocketFactory class in Commons-Net. It is very simple and you can make your own class like this and pass it to the setSocketFactory() method. This will work even with older releases of Commons-Net and you don't have to wait for the next version.

          Show
          Bogdan Drozdowski added a comment - I forgot about one possibility - you can create and install your own SocketFactory that creates Sockets with your own buffer size. Look at the DefaultSocketFactory class in Commons-Net. It is very simple and you can make your own class like this and pass it to the setSocketFactory() method. This will work even with older releases of Commons-Net and you don't have to wait for the next version.
          Hide
          Sebb added a comment -

          Thansk for the patch.

          I applied it with some minor simplifications.
          Also it seemed more sensible to set the socket size where the socket is created.

          URL: http://svn.apache.org/viewvc?rev=1361003&view=rev
          Log:
          NET-465 FTPClient setSendBufferSize and setReceiveBufferSize on data socket

          Modified:
          commons/proper/net/trunk/src/changes/changes.xml
          commons/proper/net/trunk/src/main/java/org/apache/commons/net/ftp/FTPClient.java

          Show
          Sebb added a comment - Thansk for the patch. I applied it with some minor simplifications. Also it seemed more sensible to set the socket size where the socket is created. URL: http://svn.apache.org/viewvc?rev=1361003&view=rev Log: NET-465 FTPClient setSendBufferSize and setReceiveBufferSize on data socket Modified: commons/proper/net/trunk/src/changes/changes.xml commons/proper/net/trunk/src/main/java/org/apache/commons/net/ftp/FTPClient.java
          Hide
          Chad Wilson added a comment -

          There is some concern this might have caused some rather severe performance issues in some circumstances (including the default configuration) - would one of the submitters/reviewers care to comment on NET-493?

          Show
          Chad Wilson added a comment - There is some concern this might have caused some rather severe performance issues in some circumstances (including the default configuration) - would one of the submitters/reviewers care to comment on NET-493 ?
          Hide
          Thomas Neidhart added a comment -

          The patch changed the default send/receive buffer size on the socket to 1024 which is pretty low considering the normal default, e.g. on my system

          • sendBuffer: 131072
          • receiveBuffer: 2097152

          I can understand that the slow rates are due to starvation of the higher layers based on constant buffer overruns of the underlying socket (and the involved TCP overhead of throttling the datarate). I think the idea outlined by Bogdan, to supply a custom SocketFactory would be a better solution than actually adjusting the socket buffer size in a similar way than the internal buffer.

          Another quick fix would be to not to adjust the socket buffers by default, only when the user provides another buffer size. Right now, the socket buffers are always adjusted (to the default, if there is no user-supplied value).

          Show
          Thomas Neidhart added a comment - The patch changed the default send/receive buffer size on the socket to 1024 which is pretty low considering the normal default, e.g. on my system sendBuffer: 131072 receiveBuffer: 2097152 I can understand that the slow rates are due to starvation of the higher layers based on constant buffer overruns of the underlying socket (and the involved TCP overhead of throttling the datarate). I think the idea outlined by Bogdan, to supply a custom SocketFactory would be a better solution than actually adjusting the socket buffer size in a similar way than the internal buffer. Another quick fix would be to not to adjust the socket buffers by default, only when the user provides another buffer size. Right now, the socket buffers are always adjusted (to the default, if there is no user-supplied value).
          Hide
          Sebb added a comment - - edited

          In retrospect it would have been better to provide additional getters/setters for the Socket size, as the requirements are quite different.

          It's unfortunate that the Javadoc suggests that the buffer size applies to the sockets, whereas it was originally applied to the buffered streams.

          Since the socket size is currently only set when the socket is created, a (messy) work-round with the existing code is to set the desired socket size, create the socket, and then immediately reset the size to a value more suitable for the buffered streams.

          Show
          Sebb added a comment - - edited In retrospect it would have been better to provide additional getters/setters for the Socket size, as the requirements are quite different. It's unfortunate that the Javadoc suggests that the buffer size applies to the sockets, whereas it was originally applied to the buffered streams. Since the socket size is currently only set when the socket is created, a (messy) work-round with the existing code is to set the desired socket size, create the socket, and then immediately reset the size to a value more suitable for the buffered streams.
          Hide
          Sebb added a comment -

          Reworked the original fix:

          URL: http://svn.apache.org/viewvc?rev=1437740&view=rev
          Log:
          NET-465 FTPClient setSendBufferSize and setReceiveBufferSize on data socket
          Reworked the original fix.

          Modified:
          commons/proper/net/trunk/src/changes/changes.xml
          commons/proper/net/trunk/src/main/java/org/apache/commons/net/ftp/FTPClient.java

          Show
          Sebb added a comment - Reworked the original fix: URL: http://svn.apache.org/viewvc?rev=1437740&view=rev Log: NET-465 FTPClient setSendBufferSize and setReceiveBufferSize on data socket Reworked the original fix. Modified: commons/proper/net/trunk/src/changes/changes.xml commons/proper/net/trunk/src/main/java/org/apache/commons/net/ftp/FTPClient.java
          Hide
          Sebb added a comment -

          The Apache snapshot repo should now contain the latest 3.3-SNAPSHOT code if anyone wants to give it a try.

          Feedback welcome.

          Show
          Sebb added a comment - The Apache snapshot repo should now contain the latest 3.3-SNAPSHOT code if anyone wants to give it a try. Feedback welcome.
          Hide
          Thomas Neidhart added a comment -

          I tested to download a large file (~17MB) with the FTPClientExample (default settings) from a remote server with both the 3.2 and the latest version (trunk):

          • 3.2: 2 KB/s
          • trunk: ~700 KB/s

          so it seems to work fine again.

          Thanks!

          Show
          Thomas Neidhart added a comment - I tested to download a large file (~17MB) with the FTPClientExample (default settings) from a remote server with both the 3.2 and the latest version (trunk): 3.2: 2 KB/s trunk: ~700 KB/s so it seems to work fine again. Thanks!
          Hide
          Sebb added a comment -

          OK, thanks!

          Does anyone want to test whether the new settters for SO_SNDBUF and SO_RCVBUF work OK?

          Show
          Sebb added a comment - OK, thanks! Does anyone want to test whether the new settters for SO_SNDBUF and SO_RCVBUF work OK?
          Hide
          Sebb added a comment -

          Assume it's OK. If not please re-open with details.

          Show
          Sebb added a comment - Assume it's OK. If not please re-open with details.
          Hide
          Thomas Neidhart added a comment -

          There was a comment from somebody on the user ml with some performance figures.
          The conclusion was that 3.3-SNAPSHOT was still slower than 3.1 although better than 3.2.

          Show
          Thomas Neidhart added a comment - There was a comment from somebody on the user ml with some performance figures. The conclusion was that 3.3-SNAPSHOT was still slower than 3.1 although better than 3.2.

            People

            • Assignee:
              Unassigned
              Reporter:
              Jim Kerwood
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 0.25h
                0.25h
                Remaining:
                Remaining Estimate - 0.25h
                0.25h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development