tkobayas commented on PR #7176:
URL: https://github.com/apache/incubator-kie/pull/7176#issuecomment-6077577641
@Thebas Thanks for the fix — the listener leak on delete and expiry is
clearly the right thing to address, and the tests are solid.
I found one concurrency issue with the expiry path that I think should be
fixed before merging.
### `removeFromObjectStore` accesses `dynamicFacts` without the entry-point
lock
`insert()` holds the entry-point `ReentrantLock` when it adds to
`dynamicFacts`. But `removeFromObjectStore()` is called from
`DefaultAgenda.doRetract()` during expiration, which does **not** hold that
lock. The new `removeDynamicPropertyChangeListener()` call therefore performs
`contains()` and `remove()` on the backing `HashSet` concurrently with
insertion on another thread.
A minimal fix would be to wrap the call in `removeFromObjectStore` with the
existing lock:
```java
public void removeFromObjectStore(InternalFactHandle handle) {
this.objectStore.removeHandle( handle );
ObjectTypeConf typeConf =
getObjectTypeConfigurationRegistry().getObjectTypeConf( handle.getObject() );
lock();
try {
removeDynamicPropertyChangeListener( handle, typeConf );
} finally {
unlock();
}
deleteFromTMS( handle, handle.getEqualityKey(), typeConf, null );
}
```
The `deleteStated` path already runs under the lock, so only
`removeFromObjectStore` needs the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
