RFR: 8343793: Test java/foreign/TestMemorySession.java is timing out

[Maurizio Cimadamore]

On Fri, 8 Nov 2024 12:22:54 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Hi,
>> 
>> This patch fixes the deadlock in `TestMemorySession#testAcquireCloseRace`. The lock-step spin lock is implemented as with `lock` being an `AtomicInteger`:
>> 
>>     // Keep the 2 threads operating on the same scope
>>     int k = lock.getAndAdd(1) + 1;
>>     while (k != i * 2) {
>>         Thread.onSpinWait();
>>         k = lock.get();
>>     }
>> 
>> Given the initial condition:
>> 
>>     Thread 1: i = 0
>>     Thread 2: i = 0
>>     lock: -2
>> 
>> The `lock` then undergoes the following operations:
>> 
>> 
>> 
>>         Thread 1          Thread 2         lock value
>>       getAndAdd(1)                            -1
>>                         getAndAdd(1)           0 -> Thread 2 then continues its next iteration, its i value is now 1
>>                         getAndAdd(1)           1 -> Thread 2 reaches the next iteration before thread 1 has a chance to read the value 0
>>           get()                                1 -> Thread 1 now cannot proceed because it missed the value 0
>>                            get()               1 -> Thread 2 now cannot proceed because lock can never reach 2
>> 
>> 
>> The solution is to not rely on the exact value of the lock but instead whether the lock has passed the expected value.
>> 
>> Testing: I have run this test several hundreds times and got no failure while without this patch I encountered a timeout every approximately 30 times.
>> 
>> Please take a look, thanks a lot.
>
> I'm playing with something like this:
> 
> 
> @Test
>     public void testAcquireCloseRace() throws InterruptedException {
>         int MAX_ITERATIONS = 1000;
>         AtomicInteger iteration = new AtomicInteger();
>         boolean[] result = new boolean[1];
>         MemorySessionImpl[] scopes = new MemorySessionImpl[MAX_ITERATIONS];
>         for (int i = 0; i < MAX_ITERATIONS; i++) {
>             scopes[i] = MemorySessionImpl.toMemorySession(Arena.ofShared());
>         }
> 
>         // This thread tries to close the scopes
>         Thread t1 = new Thread(() -> {
>             int it = iteration.get();
>             while (it < MAX_ITERATIONS) {
>                 while (true) {
>                     try {
>                         scopes[it].close();
>                         // if we close successfully, the iteration is now completed
>                         break;
>                     } catch (IllegalStateException e) {
>                         // scope is acquired, wait and try again
>                         Thread.onSpinWait();
>                     }
>                 }
>                 // wait for other thread to complete its iteration
>                 int prev = it;
>                 while (prev == it) {
>                     Thread.onSpinWait();
>                     it = iteration.get();
>                 }
>             }
>         });
> 
>         // This thread tries to acquire the scopes, then check if it is alive after an acquire failure
>         Thread t2 = new Thread(() -> {
>             int it = iteration.get();
>             while (it < MAX_ITERATIONS) {
>                 while (true) {
>                     try {
>                         scopes[it].acquire0();
>                     } catch (IllegalStateException e) {
>                         // we failed to acquire, meaning the other thread has closed this scope
>                         if (scopes[it].isAlive()) {
>                             result[0] = true;
>                         }
>                         // this iteration is now completed
>                         break;
>                     }
>                     // if we get here, acquire was successful, so we need to release...
>                     scopes[it].release0();
>                     // ... and then try again
>                 }
>                 // advance to next iteration
>                 it = iteration.addAndGet(1);
>             }
>         });
> 
>         t1.start();
>         t2.start();
>         t1.join();
>         t2.join();
>         assertFalse(result[0]);
>     }
> 
> 
> I think I find this logic a bit more dir...

> @mcimadamore I think your approach is more or less similar. Although it is a little bit clearer in terms of what index we are operating on, it is a bit less clear in terms of the synchronization mechanism, as it is spread out to a wider scope.

IMHO both synchronization mechanisms are spread out in scope, as no matter what you'll have an action at a distance. FWIW, I find the logic with index updating particularly hard to read -- and a very indirect way to coordinate the two threads, which is why I tried to reach for something a little more explicit.

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

PR Comment: https://git.openjdk.org/jdk/pull/21976#issuecomment-2465099538