[foreign-jextract] RFR: Minor change for possible race in shared resource list [v2]

[Maurizio Cimadamore]

On Sat, 24 Apr 2021 13:28:19 GMT, Radoslaw Smogura <github.com+7535718+rsmogura at openjdk.org> wrote:

>> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java line 142:
>> 
>>> 140:                     throw new IllegalStateException("Already closed");
>>> 141:                 }
>>> 142:                 cleanup.next = prev;
>> 
>> Why not moving this assigment inside the `if` block (e.g. when `add` succeeded) and then remove the `cleanup.next = null`  (since the assignment would never had been executed in that case) ?
>
> Do you mean something like this?
> 
> if (FST.compareAndSet(this, prev, cleanup)) {
> cleanup.next = prev;
> }
> 
> It could create race with clean block, as clean block could read and process new head, before the pointer to next has been assigned.
> 
> (I actually bit wonder, if this cleaning is required, I thought about case that this class will be kept by someone, and it can create some memory leak, but I think there's no many places where it can happen)

I guess what I was suggesting is this:


void add(ResourceCleanup cleanup) {
            while (true) {
                ResourceCleanup prev = (ResourceCleanup) FST.getAcquire(this);
                if (prev == ResourceCleanup.CLOSED_LIST) {
                    // too late
                    throw new IllegalStateException("Already closed");
                }
                if (FST.compareAndSet(this, prev, cleanup)) {
                    cleanup.next = prev;
                    return; //victory
                };
                // keep trying
            }
        }
 ```

E.g. setup `prev` only if we are sure that the element has been added to the list - but that probably won't work - as it leaves the added element in a partially invalid state.

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

PR: https://git.openjdk.java.net/panama-foreign/pull/519