On Fri, 10 May 2024 07:45:02 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix System.console().readln(null) in jshell
>>
>> Without it, jshell hangs on me. Will think of a test.
>
> 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?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596861708
