On Fri, 10 May 2024 16:41:28 GMT, Naoto Sato <[email protected]> wrote:
>> When implementing, I asked myself if I must use any of those monitors and
>> decided that I don't have to. My reasoning is below.
>>
>> `readLock`:
>>
>> - is used inside the object that `Reader reader` is initialised with, and
>>
>> - it also guards fields such as `char[] rcb`, `boolean restoreEcho` and
>> `boolean shutdownHookInstalled`.
>>
>> Since `println` and `print` don't call methods on `reader` or access the
>> above fields, they don't require `readLock`.
>>
>> `writeLock`:
>>
>> - is used inside objects that `Writer out` and `PrintWriter pw` are
>> initialised with, and
>> - also in compound operations that first write and then immediately read. (I
>> assume, it's so that no concurrent write could sneak in between writing and
>> reading parts of such a compound.)
>>
>> `println` or `print` don't call methods on `out` and certainly don't do any
>> write-read compounds. That said, they call methods on `pw`. But `pw` uses
>> `writeLock` internally. So in that sense we're covered.
>>
>> One potential concern is a write-write compound in `print`:
>>
>>
>> @Override
>> public JdkConsole print(Object obj) {
>> pw.print(obj);
>> pw.flush(); // automatic flushing does not cover print
>> return this;
>> }
>>
>>
>> I'm saying write-_write_, not write-_flush_, because as far as
>> synchronisation is concerned, `pw.flush()` should behave the same as
>> `pw.print("")`.
>>
>> While not using `writeLock` is not strictly correct, I believe the potential
>> execution interleavings with other writes are benign. What's the worst that
>> could happen? You flush more than you expected? Big deal!
>>
>> Since we exhausted all the reasons to use `writeLock`, I don't think we need
>> one.
>>
>>
>>
>> Naoto has already reviewed this PR with only minor comments. While that
>> increases my confidence in that the implementation is correct, it doesn't
>> hurt to request re-review of this specific part: @naotoj, do you think I
>> should use any of those monitors?
>
> I think your investigation is correct. As to the write-write case, there
> already is the same pattern in (`formatter` basically utilizes `pw`
> underneath)
>
> public JdkConsole format(String fmt, Object ... args) {
> formatter.format(fmt, args).flush();
> return this;
> }
>
> So I think it is acceptable.
Thank you for that explanation, Pavel. I think the crucial detail happens to be:
> But pw uses writeLock internally. So in that sense we're covered.
As you note, the same instance of `writeLock` will get used internally by the
`PrintWriter`, so I think the current version of this code is good and won't
require additionally locking in the outer code.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1598268588
