On Fri, 8 Nov 2024 12:22:54 GMT, Maurizio Cimadamore <mcimadam...@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
