Re: RFR: 8287206: Use WrongThreadException for confinement errors

2022-05-25 Thread Uwe Schindler
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

2022-05-25 Thread Maurizio Cimadamore
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

2022-05-25 Thread Maurizio Cimadamore
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

2022-05-25 Thread Uwe Schindler
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

2022-05-25 Thread Uwe Schindler
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

2022-05-24 Thread Mandy Chung
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

2022-05-24 Thread Joe Darcy
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

2022-05-24 Thread Alan Bateman
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

2022-05-24 Thread Maurizio Cimadamore
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