Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Thu, 21 Apr 2022 10:13:11 GMT, Lance Andersen wrote: > Looks fine Brian. Any thoughts as to whether a release note is warranted? Thanks, @LanceAndersen. The issue is labelled as needing a release note so you are spot on. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Looks fine Brian. Any thoughts as to whether a release note is warranted? - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Marked as reviewed by jpai (Committer). - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Oh, sorry, I had missed that synchronization had already been removed from the `InputStream` methods. Needless to say, I approve. :-) - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Thu, 21 Apr 2022 00:00:47 GMT, Stuart Marks wrote: > > I think it's a vanishingly small possibility that anything is relying on the > synchronization in these methods. Synchronization here would provide proper > memory visibility effects across threads. To use input streams from multiple > threads without interleaving operations, one would have to provide some other > means of coordination among those threads, which itself would likely ensure > proper memory visibility. I'm hard pressed to see how threads could > coordinate solely via use of the `mark` and `reset` methods. Therefore, I > think it's safe to remove synchronization from them. > > This reasoning also applies to `InputStream`. Perhaps synchronization can be > removed from there too. > > I agree that a CSR is probably warranted to capture the behavior change and > analysis. Commit f210cbf5f6bf58326a379b4b3f55fddf49d30f5c removed the synchronization from `InputStream`'s `mark()` and `reset()`; a CSR is on file. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream I think it's a vanishingly small possibility that anything is relying on the synchronization in these methods. Synchronization here would provide proper memory visibility effects across threads. To use input streams from multiple threads without interleaving operations, one would have to provide some other means of coordination among those threads, which itself would likely ensure proper memory visibility. I'm hard pressed to see how threads could coordinate solely via use of the `mark` and `reset` methods. Therefore, I think it's safe to remove synchronization from them. This reasoning also applies to `InputStream`. Perhaps synchronization can be removed from there too. I agree that a CSR is probably warranted to capture the behavior change and analysis. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 18:40:13 GMT, Alan Bateman wrote: > Thanks for the update. I can't think how anything could depend on this but > unfortunately it's been like that since early JDK releases. We will need a > release note, I'm sure Joe will give his opinion on whether a CSR should be > created. Hmm. It is true that the presence or absence of the "synchronized" modifier is not part of a method's contract. However, the multi-thread safety, or not, of a class is part of its behavioral contract. Without a lot of investigation, my current inclination is to run a CSR for this change for the (small) behavioral compatibility hazard. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Thanks for the update. I can't think how anything could depend on this but unfortunately it's been like that since early JDK releases. We will need a release note, I'm sure Joe will give his opinion on whether a CSR should be created. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods > of `java.io.FilterInputStream`. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8284930: Also remove synchronized keyword from mark() and reset() of InputStream - Changes: - all: https://git.openjdk.java.net/jdk/pull/8309/files - new: https://git.openjdk.java.net/jdk/pull/8309/files/85546c9e..f210cbf5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8309=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8309=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8309.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8309/head:pull/8309 PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset
On Wed, 20 Apr 2022 11:56:20 GMT, Jaikiran Pai wrote: > I wonder if it should be removed from InputStream at the same time. I took the presence of synchronized on those empty methods as a hint to subclasses that they too should be synchronized. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset
On Wed, 20 Apr 2022 11:56:20 GMT, Jaikiran Pai wrote: > As for the changes to `FilterInputStream`, would this change require a CSR? There might be a case where a subclass of `FilterInputStream` has overridden all other methods to add `synchronized` but has not overridden `mark` and `reset`, relying on the synchronization of the super class. I have skimmed over subclasses in the JDK, and have found no evidence of such behavior (though I have not looked exhaustively at all subclasses). Of course such subclasses could exist in the wild, but that would be both odd and fragile, since synchronization is not specified. In this particular case I don't believe that we would need a CSR to remove a synchronized keyword: whether a method is synchronized or not is not documented and not part of the spec: https://download.java.net/java/early_access/jdk19/docs/api/java.base/java/io/FilterInputStream.html#mark(int) - and `FilterInputStream` says nothing about synchronization. I agree with Alan that `InputStream` should be modified similarly. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset
On Wed, 20 Apr 2022 07:33:14 GMT, Alan Bateman wrote: > I wonder if it should be removed from InputStream at the same time. Interesting. I hadn't noticed `InputStream` had those two methods synchronized. I suspect removing synchronization from those two methods on `InputStream` is probably a lot more "simpler" (i.e. shouldn't impact any other code) since the `mark` was an empty implementation and `reset` throws an `IOException` stating mark/reset isn't supported. So if any subclasses of `InputStream` did indeed rely on mark/reset capability, then they would have already overridden these methods and dealt with any necessary synchronization themselves in those methods. So removing `synchronized` from these two methods in `InputStream`, I think is a good idea. As for the changes to `FilterInputStream`, would this change require a CSR? - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset
On Tue, 19 Apr 2022 23:26:44 GMT, Brian Burkhalter wrote: > Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods > of `java.io.FilterInputStream`. I wonder if it should be removed from InputStream at the same time. - PR: https://git.openjdk.java.net/jdk/pull/8309
RFR: 8284930: Re-examine FilterInputStream mark/reset
Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods of `java.io.FilterInputStream`. - Commit messages: - 8284930: Re-examine FilterInputStream mark/reset Changes: https://git.openjdk.java.net/jdk/pull/8309/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8309=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284930 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8309.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8309/head:pull/8309 PR: https://git.openjdk.java.net/jdk/pull/8309
