On Tue, 25 Aug 2026 12:45:47 GMT, Markus KARG <[email protected]> wrote:
>> Given the land mines, maybe it would be better to park this PR and focus
>> first on adding tests for corner cases. These tests will be needed anyway to
>> be confident with any re-implementation or changes to this code.
>
> Fast path is now enabled only for charset-created InputStreamReaders.
>
> Slow path is now handling "read() after readAllAsString()" (returns `-1`,
> does not throw). There is a test for it.
>
> I am not sure I understood your proposal correct, so kindly asking for advice:
> - Do you want me to *remove* the slow path? That would be ok for me, as my
> original proposal only had the *fast* path in mind, actually. OTOH I think it
> should be working now, and is covered with tests.
> - Do you want me to cover the slow path *with more tests*? In fact I do not
> see *which ones*.
> - Do you want me to provide a full suite of tests covering *all* corner
> cases, including externally provided CharsetDecoder? This would be a real lot
> of tests. So I added *just some* for now.
>
> You're the lead. What direction do you want me to go?
>From a quick look, the updated version looks right but it means tricky decoder
>logic is duplicated. I think it would be good to see if a simpler approach to
>just implement the fastpath in SD would be sufficient, e.g.
public String tryReadAllAsString() throws IOException {
synchronized (lock) {
ensureOpen();
if (in != null && decoderFromCharset && !readCalled) {
return new String(in.readAllBytes(), cs);
}
}
return null;
}
Overall control would remaining in the enclosing InputStreamReader where the
readAllAsString override would be something like this:
@Override
public String readAllAsString() throws IOException {
String s = sd.tryReadAllAsString();
return (s != null) ? s : super.readAllAsString();
}
Would you mind trying that direction? My guess (and I don't have evidence) is
that the users of readAllAsString are looking to slurp the entire content
rather than then remaining content, maybe a static analysis would suggestion
otherwise.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32264#discussion_r3854530793
