On Wed, 21 Jun 2023 14:27:26 GMT, Chen Liang <li...@openjdk.org> wrote:
>> src/java.base/share/classes/java/io/PipedOutputStream.java line 166:
>>
>>> 164: @Override
>>> 165: public synchronized void flush() throws IOException {
>>> 166: PipedInputStream sink = this.sink;
>>
>> Suggestion:
>>
>> var sink = this.sink;
>>
>> As seen in other methods.
>
> On second thought, this is probably not necessary; write to the sink field is
> in another synchronized method, and this method is synchronized already. Is
> the goal here to remove the synchronized on flush?
Good observation. Removing `synchronized` on flush might be a worthwhile goal
but possible side effects (including on potential subclasses) should be
carefully considered.
I support stashing `sink` in a local variable though, even if the pointer can't
be concurrently modified, just to make it clear that we only have one volatile
read.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14589#discussion_r1237152608
