On Thu, 27 Aug 2026 17:41:03 GMT, Jaikiran Pai <[email protected]> wrote:
>> The clearest place to set readCalled is as close as possible to where read
>> is called, so read and read0 after acquiring lock.
>
> While at it, both `in` and `decoderFromCharset` are `final` fields. I think
> we should move the check for these fields out of the synchronized block in
> this method. Something like (this untested code):
>
>
> if (in == null || !decoderFromCharset) {
> return null;
> }
> synchronized (lock) {
> ensureOpen();
> if (!readCalled) {
> return new String(in.readAllBytes(), cs);
> }
> }
> return null;
>
> That would prevent the necessity of acquiring the monitor on `lock` for some
> cases.
Adopted in latest commit of this PR.
Pulling the `readCalled = true;` line up into `read()` as suggested by Alan
implied adding `synchronized(this)` in `read()`, as seen in latest commit of
this PR. As a side effect we now could remove `synchronized(this)` from
`read0()` in turn, if we like, as all callers are synchronized now themselves.
Agreed?
Also, `readCalled = true;` was needed in *more* places than just `read()` and
`read0()` after removing it from `readBytes()`, as there are several
non-private entry points (like `implRead()`) that other classes could call.
IMHO the code was simpler before (when there was just one single place that did
`readCalled = true;`). It's up to the reviewers to decide, but my personal
opinion is that `readCalled = true;` should not tell us whether some
`StreamDecoder:*read*()` method was called (which is several locations), but
solely whether `in.read()` was called (which is just a single location).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32264#discussion_r3881780325
