Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. I changed the PR on our side, `IllegalStateException` is fine, if you catch it as close as possible to the code calling varhandles and so on (which is the case for Lucene). Thanks for this PR, makes life much easier! - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Wed, 25 May 2022 10:16:54 GMT, Maurizio Cimadamore wrote: > > Thinking more about it: `IllegalStateException` is fine for a closed > > `MemorySession`. If used by wrong thread the exception was indeed > > confusing. Now that the abiguity is gone, I can change our (Lucene) code > > and just transform every ISE to an already closed in our code. I just > > wanted to bring up that issue here. > > PR is here: [apache/lucene#912](https://github.com/apache/lucene/pull/912) > > I've been thinking something similar. I'd suggest to keep the API as is, and > maybe revise it at a later point if lack of a specific exception for the > "already closed" case proves to be too cumbersome to workaround. Basically, with this patch you only get ISE if you are accessing _in a moment in time_ when you are not supposed to. Similarly, when calling `close`, you get ISE if you are closing a segment that is in a bad state (e.g. already closed, or temporarily locked by some native call). This feels consistent. The confinement exception was the confounding factor, I think, and a lot of checks against the exception message came from there. - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. > Thinking more about it: `IllegalStateException` is fine for a closed > `MemorySession`. If used by wrong thread the exception was indeed confusing. > Now that the abiguity is gone, I can change our (Lucene) code and just > transform every ISE to an already closed in our code. I just wanted to bring > up that issue here. > > PR is here: [apache/lucene#912](https://github.com/apache/lucene/pull/912) I've been thinking something similar. I'd suggest to keep the API as is, and maybe revise it at a later point if lack of a specific exception for the "already closed" case proves to be too cumbersome to workaround. - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. Thinking more about it: `IllegalStateException` is fine for a closed `MemorySession`. If used by wrong thread the exception was indeed confusing. Now that the abiguity is gone, I can change our (Lucene) code and just transform every ISE to an already closed in our code. I just wanted to bring up that issue here. PR is here: https://github.com/apache/lucene/pull/912 - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore
wrote:
> This patch tweaks the foreign API to use the newly added
> `WrongThreadException` instead of `IllegalStateException` to report
> confinement errors.
I have seen this PR now: Would it be also a good idea to replace the
`IllegalStateException` for already closed `MemorySession` by some special
exception? In Lucene we catch `IllegalStateException` in our IO layer and check
if message contains "closed", example:
-
https://github.com/apache/lucene/blob/d182134bf88a44bf76ebd1c1d40b225ecdca1f4b/lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java#L137
(catch)
-
https://github.com/apache/lucene/blob/d182134bf88a44bf76ebd1c1d40b225ecdca1f4b/lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java#L93-L103
(that's the hack)
The check using `ex.getMessage().contains("closed")` feels bad, especially if
it may be translated to a locale.
-
PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. javadoc: http://cr.openjdk.java.net/~mcimadamore/8287206/v1/javadoc/java.base/module-summary.html specdiff: http://cr.openjdk.java.net/~mcimadamore/8287206/v1/specdiff_out/overview-summary.html - PR: https://git.openjdk.java.net/jdk/pull/8865
RFR: 8287206: Use WrongThreadException for confinement errors
This patch tweaks the foreign API to use the newly added `WrongThreadException` instead of `IllegalStateException` to report confinement errors. - Commit messages: - Merge branch 'master' into wrong_thread_ex - Initial push Changes: https://git.openjdk.java.net/jdk/pull/8865/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8865=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287206 Stats: 254 lines in 12 files changed: 150 ins; 1 del; 103 mod Patch: https://git.openjdk.java.net/jdk/pull/8865.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8865/head:pull/8865 PR: https://git.openjdk.java.net/jdk/pull/8865
