Re: RFR: 8305457: Implement java.io.IO [v7]
On Fri, 10 May 2024 16:41:28 GMT, Naoto Sato 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
Re: RFR: 8305457: Implement java.io.IO [v7]
On Fri, 10 May 2024 14:54:10 GMT, Pavel Rappo 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
Re: RFR: 8305457: Implement java.io.IO [v7]
On Fri, 10 May 2024 07:45:02 GMT, Jaikiran Pai 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
Re: RFR: 8305457: Implement java.io.IO [v7]
On Fri, 10 May 2024 07:23:38 GMT, Per Minborg 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/java/io/IO.java line 47:
>
>> 45:
>> 46: private IO() {
>> 47: throw new Error("no instances");
>
> Is this necessary?
No, it's not necessary.
My guess is that throwing an unconditional error (usually with a snarky
message) from the inside the sole private constructor is a historical idiom to
deter deep reflectors with their `setAccessible` and what not. Maybe that idiom
is outdated with modules and recent efforts on cranking up the
[integrity](https://openjdk.org/jeps/8305968), that I don't know.
FWIW, here's a snippet[^*] from "Item 4: Enforce noninstantiability with a
private constructor" you referred to in your earlier comment:
> A default constructor is generated only if a class contains no explicit
> constructors, so a class can be made noninstantiable by including a private
> constructor:
>
>// Noninstantiable utility class
>public class UtilityClass {
>// Suppress default constructor for noninstantiability
>private UtilityClass() {
>throw new AssertionError();
>}
>... // Remainder omitted
>}
>
> Because the explicit constructor is private, it is inaccessible outside of
> the class. The `AssertionError` isn’t strictly required, but it provides
> insurance in case the constructor is accidentally invoked from within the
> class. It guarantees the class will never be instantiated under any
> circumstances.
[^*]: I created that snippet from the second edition, whose page 19 is
conveniently publicly available here:
https://www.google.ie/books/edition/Effective_Java/ka2VUBqHiWkC?hl=en&gbpv=1&dq=Item+4:+Enforce+noninstantiability+with+a+private+constructor&pg=PA19&printsec=frontcover
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596615677
Re: RFR: 8305457: Implement java.io.IO [v7]
On Fri, 10 May 2024 07:29:42 GMT, Per Minborg wrote: >> If the sole constructor of a class is private, not only does it preclude the >> class from being instantiated, but it also precludes the class from being >> extended. Mind you, even with the sole private constructor, both >> instantiation and extension are still possible from inside the class or its >> nested classes. As long as we don't leak such instances to API clients, they >> won't seem to be able to observe any difference between "the private >> constructor" and "the private constructor of a final class". >> >> I think that just having that constructor private but the class "non-final" >> makes bigger bang for the buck. We can defer the decision to make the class >> final. >> >> If this helps, here are a few well-known similar classes: >> >> - java.util.Collections >> - java.util.concurrent.Executors > > Although true (and also described in Item 4 in "Effective Java" 3rd ed by JB) > I think it is better to also signal the intent by marking the class as > `final`. See for example Arrays that we updated some time ago. It feels better to specify what the class is for than to "signal" all the things it is not for. In the first sentence of its documentation comment, `IO` says that it's a "collection of static convenience methods". I'm not sure how a class can signal its intent more clearly than that. Like that of `Arrays`, `Collections`, `Collectors`, `Comparators`, `Executors`, `Files`, `Objects`, etc., an instance of `IO` or of its subclass is no more useful than an instance of `Object`. Even if `IO` isn't `final` and has a default constructor, it doesn't matter in practice. Not too far ago, we started generating warnings on default constructors. To avoid those warnings, I had to provide an explicit constructor. While unnecessary, I'll make `IO` `final`. Maybe it will help an IDE tell the programmer that they are doing something meaningless. - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596572570
Re: RFR: 8305457: Implement java.io.IO [v7]
On Thu, 9 May 2024 18:27:08 GMT, Pavel Rappo wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> 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?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596392802
Re: RFR: 8305457: Implement java.io.IO [v7]
On Tue, 7 May 2024 20:23:11 GMT, Pavel Rappo wrote: >>> Should this be final? >> >> With only the private constructor, it doesn't matter too much in practice, >> but an explicit `final` would be good documentation if that is the intent. > > If the sole constructor of a class is private, not only does it preclude the > class from being instantiated, but it also precludes the class from being > extended. Mind you, even with the sole private constructor, both > instantiation and extension are still possible from inside the class or its > nested classes. As long as we don't leak such instances to API clients, they > won't seem to be able to observe any difference between "the private > constructor" and "the private constructor of a final class". > > I think that just having that constructor private but the class "non-final" > makes bigger bang for the buck. We can defer the decision to make the class > final. > > If this helps, here are a few well-known similar classes: > > - java.util.Collections > - java.util.concurrent.Executors Although true (and also described in Item 4 in "Effective Java" 3rd ed by JB) I think it is better to also signal the intent by marking the class as `final`. See for example Arrays that we updated some time ago. - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596377720
Re: RFR: 8305457: Implement java.io.IO [v7]
On Thu, 9 May 2024 18:27:08 GMT, Pavel Rappo wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> 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/java/io/IO.java line 47:
> 45:
> 46: private IO() {
> 47: throw new Error("no instances");
Is this necessary?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596370177
Re: RFR: 8305457: Implement java.io.IO [v7]
On Thu, 9 May 2024 18:27:08 GMT, Pavel Rappo wrote: >> Please review this PR which introduces the `java.io.IO` top-level class and >> three methods to `java.io.Console` for [Implicitly Declared Classes and >> Instance Main Methods (Third Preview)]. >> >> This PR has been obtained as `git merge --squash` of a now obsolete [draft >> PR]. >> >> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: >> https://bugs.openjdk.org/browse/JDK-8323335 >> [draft PR]: https://github.com/openjdk/jdk/pull/18921 > > 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. Marked as reviewed by smarks (Reviewer). Latest changes look good. - PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2049241670 PR Comment: https://git.openjdk.org/jdk/pull/19112#issuecomment-2103859341
Re: RFR: 8305457: Implement java.io.IO [v7]
> Please review this PR which introduces the `java.io.IO` top-level class and > three methods to `java.io.Console` for [Implicitly Declared Classes and > Instance Main Methods (Third Preview)]. > > This PR has been obtained as `git merge --squash` of a now obsolete [draft > PR]. > > [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: > https://bugs.openjdk.org/browse/JDK-8323335 > [draft PR]: https://github.com/openjdk/jdk/pull/18921 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. - Changes: - all: https://git.openjdk.org/jdk/pull/19112/files - new: https://git.openjdk.org/jdk/pull/19112/files/46a7af1f..1bf7a4cd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19112&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19112&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19112.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19112/head:pull/19112 PR: https://git.openjdk.org/jdk/pull/19112
