On Fri, 10 May 2024 16:41:28 GMT, Naoto Sato <na...@openjdk.org> 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