On Fri, 10 May 2024 14:54:10 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java line 61: >> >>> 59: @Override >>> 60: public JdkConsole println(Object obj) { >>> 61: pw.println(obj); >> >> Are these `println(...)` and `print(...)` methods intentionally not using a >> `writeLock` unlike the `readln(...)` and `readLine(...)` methods which do >> use (read and write) locks? > > 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596976499