Hi,
I found a thread-safety bug in PipedReader.close(). Unlike
PipedInputStream.close(), it modifies shared state without any
synchronization or volatile declaration.
The bug:
PipedReader.close() (current code):
public void close() throws IOException {
in = -1; // no synchronization
closedByReader = true; // not volatile, no synchronization
}
PipedInputStream.close() (correct pattern):
public void close() throws IOException {
closedByReader = true; // volatile field
synchronized (this) {
in = -1;
}
}
Two differences:
1. PipedReader.closedByReader is not volatile (line 39), while
PipedInputStream.closedByReader is volatile (line 54)
2. PipedReader.close() has no synchronized block, while
PipedInputStream.close() wraps in = -1 in synchronized(this)
This matters because PipedWriter.connect() reads closedByReader
while holding the PipedWriter's lock (not PipedReader's lock), so
without volatile, it may see a stale value. Also, receive() and
read() are synchronized methods that check closedByReader and in
— without synchronization in close(), these state changes may not
be visible.
Reproducer:
A concurrent stress test shows ~77% of writes succeed after close():
PipedWriter pw = new PipedWriter();
PipedReader pr = new PipedReader(pw);
pw.write('x');
// Thread 1: // Thread 2:
pr.close(); pw.write('y'); // succeeds ~77% of the time!
The fix:
Align PipedReader with PipedInputStream:
1. Declare closedByReader as volatile
2. Reorder close() to set volatile flag first, then synchronized(this) { in =
-1; }
volatile boolean closedByReader = false;
...
public void close() throws IOException {
closedByReader = true;
synchronized (this) {
in = -1;
}
}
The patch includes a jtreg test that verifies the volatile declaration
via reflection, tests synchronization behavior under lock contention,
and runs a 5000-iteration concurrent close/write stress test.
Webrev:
https://github.com/wenshao/jdk/tree/fix/piped-reader-close-synchronization
<https://github.com/wenshao/jdk/tree/fix/piped-reader-close-synchronization >
Thanks,
Shaojin Wen
