Commons Net
  1. Commons Net
  2. NET-466

Regression: TelnetInputStream#available() blocks

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 3.1
    • Fix Version/s: 3.2
    • Component/s: Telnet
    • Labels:
      None

      Description

      When testing with Commons Net 3.1 for Eclipse https://bugs.eclipse.org/bugs/show_bug.cgi?id=194473 , I found that our telnet client blocks when the ReaderThread is running and waiting for new data. Investigation shows that our code blocks on TelnetInputStream#available().

      This regression is due to the code introduced for NET-437 "TelnetInputStream doesn't support non-blocking IO when reader thread is not enabled":

      TelnetInputStream#available() now calls super.available() which is declared as "synchronized" on BufferedInputStream in JDK 1.6.0_21 at least. But at the same time, the telnet ReaderThread has already the Monitor on BufferedInputStream and doesn't give it up while sitting in read0().

      This seems to be exactly the situation that the comment before TelnetInputStream#close() warns about:
      // Cannot be synchronized. Will cause deadlock if run() is blocked
      // in read because BufferedInputStream read() is synchronized.

      This is a severe issue since it violates the specification and use of available().

        Issue Links

          Activity

          Hide
          Gary Gregory added a comment -

          Are you saying that all methods in BufferedInputStream marked with synchronized should also be marked as such in TelnetInputStream?

          This in TelnetClient is confusing:

          protected void _connectAction_() throws IOException
              {
                  super._connectAction_();
                  TelnetInputStream tmp = new TelnetInputStream(_input_, this, readerThread);
                  if(readerThread)
                  {
                      tmp._start();
                  }
                  // __input CANNOT refer to the TelnetInputStream.  We run into
                  // blocking problems when some classes use TelnetInputStream, so
                  // we wrap it with a BufferedInputStream which we know is safe.
                  // This blocking behavior requires further investigation, but right
                  // now it looks like classes like InputStreamReader are not implemented
                  // in a safe manner.
                  __input = new BufferedInputStream(tmp);
                  __output = new TelnetOutputStream(this);
              }
          

          TelnetInputStream subclasses BufferedInputStream and this method wraps the TelnetInputStream in a BufferedInputStream. Is that what is intended here? If so, it needs better docs!

          Show
          Gary Gregory added a comment - Are you saying that all methods in BufferedInputStream marked with synchronized should also be marked as such in TelnetInputStream ? This in TelnetClient is confusing: protected void _connectAction_() throws IOException { super ._connectAction_(); TelnetInputStream tmp = new TelnetInputStream(_input_, this , readerThread); if (readerThread) { tmp._start(); } // __input CANNOT refer to the TelnetInputStream. We run into // blocking problems when some classes use TelnetInputStream, so // we wrap it with a BufferedInputStream which we know is safe. // This blocking behavior requires further investigation, but right // now it looks like classes like InputStreamReader are not implemented // in a safe manner. __input = new BufferedInputStream(tmp); __output = new TelnetOutputStream( this ); } TelnetInputStream subclasses BufferedInputStream and this method wraps the TelnetInputStream in a BufferedInputStream . Is that what is intended here? If so, it needs better docs!
          Hide
          Konrad Garus added a comment -

          I have not inspected the source, just wanted to report/confirm that TelnetInputStream.available() seems to be blocking indefinitely. I noticed this regression after upgrading from 1.4.1 to 3.1.

          It's a severe issue for us, blocking update which we direly need for other areas (FTP client).

          Show
          Konrad Garus added a comment - I have not inspected the source, just wanted to report/confirm that TelnetInputStream.available() seems to be blocking indefinitely. I noticed this regression after upgrading from 1.4.1 to 3.1. It's a severe issue for us, blocking update which we direly need for other areas (FTP client).
          Hide
          Konrad Garus added a comment -

          The issue disappears as soon as I disable the reader thread.

          Here are relevant stacktraces from the lock-up:

          "Thread-0" daemon prio=10 tid=0x00007f721c61a000 nid=0x3dea runnable [0x00007f7218ee7000]
             java.lang.Thread.State: RUNNABLE
          	at java.net.SocketInputStream.socketRead0(Native Method)
          	at java.net.SocketInputStream.read(SocketInputStream.java:129)
          	at java.io.BufferedInputStream.read1(BufferedInputStream.java:256)
          	at java.io.BufferedInputStream.read(BufferedInputStream.java:317)
          	- locked <0x00000007d70f7510> (a java.io.BufferedInputStream)
          	at java.io.BufferedInputStream.fill(BufferedInputStream.java:218)
          	at java.io.BufferedInputStream.read(BufferedInputStream.java:237)
          	- locked <0x00000007d70fbab0> (a org.apache.commons.net.telnet.TelnetInputStream)
          	at org.apache.commons.net.telnet.TelnetInputStream.__read(TelnetInputStream.java:140)
          	at org.apache.commons.net.telnet.TelnetInputStream.run(TelnetInputStream.java:608)
          	at java.lang.Thread.run(Thread.java:662)
          
             Locked ownable synchronizers:
          	- None
          
          
          
          "main" prio=10 tid=0x00007f721c00d000 nid=0x3dd0 waiting for monitor entry [0x00007f7223c29000]
             java.lang.Thread.State: BLOCKED (on object monitor)
          	at java.io.BufferedInputStream.available(BufferedInputStream.java:381)
          	- waiting to lock <0x00000007d70fbab0> (a org.apache.commons.net.telnet.TelnetInputStream)
          	at org.apache.commons.net.telnet.TelnetInputStream.available(TelnetInputStream.java:563)
          	- locked <0x00000007d70fe320> (a [I)
          	at java.io.BufferedInputStream.available(BufferedInputStream.java:381)
          	- locked <0x00000007d7100498> (a java.io.BufferedInputStream)
          	at com.example.Main.main(Main.java:95)
          
             Locked ownable synchronizers:
          	- None
          
          Show
          Konrad Garus added a comment - The issue disappears as soon as I disable the reader thread. Here are relevant stacktraces from the lock-up: " Thread -0" daemon prio=10 tid=0x00007f721c61a000 nid=0x3dea runnable [0x00007f7218ee7000] java.lang. Thread .State: RUNNABLE at java.net.SocketInputStream.socketRead0(Native Method) at java.net.SocketInputStream.read(SocketInputStream.java:129) at java.io.BufferedInputStream.read1(BufferedInputStream.java:256) at java.io.BufferedInputStream.read(BufferedInputStream.java:317) - locked <0x00000007d70f7510> (a java.io.BufferedInputStream) at java.io.BufferedInputStream.fill(BufferedInputStream.java:218) at java.io.BufferedInputStream.read(BufferedInputStream.java:237) - locked <0x00000007d70fbab0> (a org.apache.commons.net.telnet.TelnetInputStream) at org.apache.commons.net.telnet.TelnetInputStream.__read(TelnetInputStream.java:140) at org.apache.commons.net.telnet.TelnetInputStream.run(TelnetInputStream.java:608) at java.lang. Thread .run( Thread .java:662) Locked ownable synchronizers: - None "main" prio=10 tid=0x00007f721c00d000 nid=0x3dd0 waiting for monitor entry [0x00007f7223c29000] java.lang. Thread .State: BLOCKED (on object monitor) at java.io.BufferedInputStream.available(BufferedInputStream.java:381) - waiting to lock <0x00000007d70fbab0> (a org.apache.commons.net.telnet.TelnetInputStream) at org.apache.commons.net.telnet.TelnetInputStream.available(TelnetInputStream.java:563) - locked <0x00000007d70fe320> (a [I) at java.io.BufferedInputStream.available(BufferedInputStream.java:381) - locked <0x00000007d7100498> (a java.io.BufferedInputStream) at com.example.Main.main(Main.java:95) Locked ownable synchronizers: - None
          Hide
          Sebb added a comment -

          Applied a fix which should work:

          URL: http://svn.apache.org/viewvc?rev=1374548&view=rev
          Log:
          NET-466 Regression: TelnetInputStream#available() blocks

          Modified:
          commons/proper/net/trunk/src/changes/changes.xml
          commons/proper/net/trunk/src/main/java/org/apache/commons/net/telnet/TelnetInputStream.java

          It might be that synchronizing the methods as per Gary's comment would also fix the issue.

          Does anyone have a suitable test case that shows the issue?

          Show
          Sebb added a comment - Applied a fix which should work: URL: http://svn.apache.org/viewvc?rev=1374548&view=rev Log: NET-466 Regression: TelnetInputStream#available() blocks Modified: commons/proper/net/trunk/src/changes/changes.xml commons/proper/net/trunk/src/main/java/org/apache/commons/net/telnet/TelnetInputStream.java It might be that synchronizing the methods as per Gary's comment would also fix the issue. Does anyone have a suitable test case that shows the issue?
          Hide
          Jos P added a comment -

          We had problems using the Telnet client in 3.1 with the 'available' that blocked (every time again) So we reverted to 3.0.1. I now downloaded the source code from the trunk and tried it again and now it just works. Don't have a real test case because it is rather interwoven in the code. So not a "scientific" test, rather a practical one.

          Show
          Jos P added a comment - We had problems using the Telnet client in 3.1 with the 'available' that blocked (every time again) So we reverted to 3.0.1. I now downloaded the source code from the trunk and tried it again and now it just works. Don't have a real test case because it is rather interwoven in the code. So not a "scientific" test, rather a practical one.
          Hide
          Sebb added a comment -

          Thanks, that's very useful to know.

          Show
          Sebb added a comment - Thanks, that's very useful to know.

            People

            • Assignee:
              Unassigned
              Reporter:
              Martin Oberhuber
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development