[foreign-memaccess+abi] RFR: 8317514: Ensure MemorySegment is initialized before touching NativeMemorySegmentImpl

[Jorn Vernee]

On Mon, 9 Oct 2023 18:22:13 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Please review this fix for a class initializer deadlock.
>> 
>> The issue is that `MemorySegment` references `NativeMemorySegmentImpl` in its initializer, through the `NULL` constant. `NativeMemorySegmentImpl` also depends on `MemorySegment` to be initialized, since it's a super interface with default methods. The VM does not drop the initialization monitor of `NativeMemorySegmentImpl` while trying to initialize super classes/interfaces (as mandated by the JVM spec).
>> 
>> When 2 threads try to intialize `MemorySegment` (e.g. through `ofAddress`) and `NativeMemorySegmentImpl` (e.g. through Arena::allocateFrom), at the same time, a deadlock can occur.
>> 
>> This PR fixes this issue by ensuring `MemorySegment` is initialized in all cases where we directly access `NativeMemorySegmentImpl` or `MappedMemorySegmentImpl`.
>> 
>> We might consider a fix that removes the cycle in the future as well.
>
> src/java.base/share/classes/jdk/internal/foreign/Utils.java line 59:
> 
>> 57:     public static final boolean IS_WINDOWS = privilegedGetProperty("os.name").startsWith("Windows");
>> 58: 
>> 59:     static {
> 
> Another possibility here is to make sure that the static methods here (and also in `NativeMemorySegmentImpl` go via `reinterpret`, so that there's nothing additional to do.

Yes, that's also an option, but `reintepret` is CallerSensitive, so this does introduce some overhead in terms of startup perf I think.

At the same time, it seems to require a similar amount of care to be taken. So I'm not sure how much it's an improvement over `ensureClassInitialized`.

> test/jdk/java/foreign/TestDeadlock.java line 51:
> 
>> 49:         });
>> 50: 
>> 51:         Thread t2 = Thread.ofPlatform().start(() -> {
> 
> We do not seem to test:
> * mapped segments
> * symbol lookup segments
> * linker-generated segments
> 
> Is that ok?

I'll try to add a test for mapped segments as well (through FileChannel::map). Though, it might not be possible to reproduce an issue in a test. 

But, the other two do not touch NativeMemorySegmentImpl or MappedMemorySegmentImpl directly, so I'm not sure. Theoretically we can test all the API points (that create MemorySegments), but given the tricky timing (the current test has to carefully pre-initialize several classes to get there), I'm not sure it's useful to spend resources on running those tests.

-------------

PR Review Comment: https://git.openjdk.org/panama-foreign/pull/902#discussion_r1350677668
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/902#discussion_r1350675117