On Wed, 9 Sep 2026 13:30:25 GMT, Jaikiran Pai <[email protected]> wrote:
>> Thank you for your experiments, Jaikiran! Suspending myself until you're
>> done.
>
> Hello Markus, I've run the experiments that I had in mind. I am a bit
> preoccupied with a few other things and will need some more days to make to a
> conclusion of those experiments.
I have gone through the usages of `InputStreamReader` and very specifically
sub-classes of `InputStreamReader` which override the `read` methods. The
`InputStreamReader` class and its read() methods (which take different
parameters) have been around for several decades. In the experiment results, I
see that there aren't too many sub-classes of this class that override these
read methods. However, there are some library classes which override these
methods and have some non-trivial code in those implementations. For example,
in some cases, after calling `super.read(...)` these overridden methods do
validations against the decoded characters to either report errors (for some
specific characters) or to replace the read character with some pre-defined
replacement character.
In the current implementation of `InputStreamReader` in JDK mainline, it is
guaranteed that any read operation, be it through the read(...) methods or
`readAllAsString()` or `readAllLines()` or even `skip()`, the sub-class'
read(...) method will be invoked and it will have a chance to run the
library/application specific code. However, with the proposed change in this PR
we now have a situation that the `readAllAsString()` implementation may no
longer call any of the read(...) methods of the sub-class and instead would
directly return a `String` constructed out of the decoded characters.
`readAllAsString()` is a newly introduced method (in Java 25) and it's very
likely that no sub-class of `InputStreamReader` overrides it currently (in
fact, in the usage search I ran, I found no such override which isn't a
surprise). What that then means is that, for such sub-class of
`InputStreamReader` constructed using a `Charset` with the new proposed
implementation, it's now possible for application code to see a different
string value against the same underlying byte stream depending on whether the
code calls `readAllAsString()` or some other read method.
For example, consider this code:
import java.io.*;
public class Bar {
private static final String INPUT = "hello,world";
public static void main(final String[] args) throws IOException {
final String expected = "hello/world"; // we expect the SomeReader to
replace "," with "/"
// readAllLines to read the underlying stream and decode the bytes to
characters
final String firstLine =
newInputStreamReader().readAllLines().getFirst();
if (!expected.equals(firstLine)) {
throw new AssertionError("FAILED: unexpected result from
readAllLines: " + firstLine + " expected: " + expected);
}
System.out.println("PASSED: readAllLines() returned " + firstLine);
// readAllAsString to read the underlying stream and decode the bytes
to characters
final String allAsString = newInputStreamReader().readAllAsString();
if (!expected.equals(allAsString)) {
throw new AssertionError("FAILED: unexpected result from
readAllAsString: " + allAsString + " expected: " + expected);
}
System.out.println("PASSED: readAllAsString() returned " + allAsString);
}
private static Reader newInputStreamReader() {
return new SomeReader(new ByteArrayInputStream(INPUT.getBytes()));
}
private static final class SomeReader extends InputStreamReader {
private SomeReader(InputStream in) {
super(in);
}
@Override
public int read(char[] cbuf, int off, int len) throws IOException {
int n = super.read(cbuf, off, len);
if (n == -1) {
return -1;
}
for (int i = 0; i < n; i++) {
if (cbuf[off + i] == ',') {
cbuf[off + i] = '/';
}
}
return n;
}
}
}
What this tries to demonstrate is that depending on which API of Reader gets
used by the application, the same underlying byte stream would now generate a
different sequence of characters for such sub-classes. When run against the
proposed PR, the application fails due to unexpected output from the
readAllAsString() method.
Given this, I think skipping the call to the read(...) method of the
`InputStreamReader` sub-class only from the readAllAsString() would introduce
too big an inconsistency. Perhaps we should just add this proposed optimization
only to instances which are of exact `InputStreamReader` type? So something
like this (untested) change in `InputStreamReader`:
@Override
public String readAllAsString() throws IOException {
// optimized implementation for InputStreamReader to
// try and avoid unnecessary buffer copies in super.readAllAsString()
if (this.getClass() == InputStreamReader.class) {
String s = sd.tryReadAllAsString();
return (s != null) ? s : super.readAllAsString();
}
return super.readAllAsString();
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32264#discussion_r4029014544
