You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by df...@apache.org on 2004/01/09 07:19:23 UTC
cvs commit: jakarta-commons/net/src/java/org/apache/commons/net/telnet TelnetInputStream.java
dfs 2004/01/08 22:19:23
Modified: net/src/java/org/apache/commons/net/telnet
TelnetInputStream.java
Log:
"Jeff Barrett" <JB...@sawyermedia.com> reports that:
I'm using commons-net to ftp a bunch of files around every night as a batch process. It probably copies just under 1g. Over the course of that process I'll often get 2 or 3 thrown exceptions like so:
[java] java.lang.NullPointerException
[java] at java.io.BufferedInputStream.read(BufferedInputStream.java:279)
[java] at java.io.BufferedInputStream.fill(BufferedInputStream.java:183)
[java] at java.io.BufferedInputStream.read(BufferedInputStream.java:201)
[java] at org.apache.commons.net.telnet.TelnetInputStream.__read(TelnetInputStream.java:140)
[java] at org.apache.commons.net.telnet.TelnetInputStream.run(TelnetInputStream.java:464)
[java] at java.lang.Thread.run(Thread.java:536)
Since we didn't open a Bugzilla report, I'm recording my email
explanation as a log message:
That's all happening in BufferedInputStream. Looking at the code
for BufferedInputStream in J2SE 1.4.2, it looks like the
NullPointerException exception is happening at a call of in.available().
in is set to null in close(). So it seems there may be a situation
whereby a read by TelnetInputStream.__read is being performed after
a close. Looking at the code, I'm pretty sure there's a problem.
TelnetInputStream calls super.close() BEFORE entering a
synchronized block. There's a race condition that could allow
the reader thread to start a read after BufferedInputStream.close()
is called. The reason it hardly ever happns is that
BufferedInputStream calls an ensureOpen() method at the
beginning of read() that checks to make sure in is not null.
However, between that call and the call to in.available(),
the value may become null because BufferdInputStream.close()
is not a synchronized method, unlike BufferedInputStream.read().
Now, there's a comment in the code before the close
call that I'm guilty of that says:
// Completely disregard the fact thread may still be running.
// We can't afford to block on this close by waiting for
// thread to terminate because few if any JVM's will actually
// interrupt a system read() from the interrupt() method.
This means the close() is being used to force the read() to return -1
or throw an exception. If we synchronize the call to close, we
may get deadlock. Mind you, this is circa JDK 1.1.x.
The good news is that the NullPointerException is probably harmless
because it's happening when the stream is being closed and the thread
is terminated. Also, it's the FTP control stream, not the data stream
that's being affected, so there will be no loss of data. The bad
news is that I'm not immediately sure how to fix it in light of the
code comment. This is a great example of why I'd like to get rid of
the threads and move to java.nio for version 2.0.
I can't come up with any ideas that don't run the risk of
deadlock on some JVM. I'm afraid the right fix may be to
simply catch any RuntimeExceptions thrown by read and take
the same action as a close(). We probably should have been
doing that anyway to gracefully handle unexpected situations.
As an interim fix, I applied that change.
Revision Changes Path
1.9 +20 -4 jakarta-commons/net/src/java/org/apache/commons/net/telnet/TelnetInputStream.java
Index: TelnetInputStream.java
===================================================================
RCS file: /home/cvs/jakarta-commons/net/src/java/org/apache/commons/net/telnet/TelnetInputStream.java,v
retrieving revision 1.8
retrieving revision 1.9
diff -u -r1.8 -r1.9
--- TelnetInputStream.java 2 Jan 2004 03:39:06 -0000 1.8
+++ TelnetInputStream.java 9 Jan 2004 06:19:23 -0000 1.9
@@ -439,7 +439,7 @@
synchronized (__queue)
{
__hasReachedEOF = true;
- __isClosed = true;
+ __isClosed = true;
if (__thread.isAlive())
{
@@ -481,6 +481,14 @@
}
continue;
}
+ } catch(RuntimeException re) {
+ // We treat any runtime exceptions as though the
+ // stream has been closed. We close the
+ // underlying stream just to be sure.
+ super.close();
+ // Breaking the loop has the effect of setting
+ // the state to closed at the end of the method.
+ break _outerLoop;
}
// Critical section because we're altering __bytesAvailable,
@@ -515,19 +523,27 @@
}
}
}
- catch (IOException e)
+ catch (IOException ioe)
{
synchronized (__queue)
{
- __ioException = e;
+ __ioException = ioe;
}
}
synchronized (__queue)
{
- __isClosed = true; // Possibly redundant
+ __isClosed = true; // Possibly redundant
__hasReachedEOF = true;
__queue.notify();
}
}
}
+
+/* Emacs configuration
+ * Local variables: **
+ * mode: java **
+ * c-basic-offset: 4 **
+ * indent-tabs-mode: nil **
+ * End: **
+ */
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org