Fix issue: #525: A race condition in styx object store. #537
Conversation
mikkokar
changed the title
| pendingChangeNotification.set(false) | ||
|
|
||
| issuedSnapshot = pendingSnapshot |
There was a problem hiding this comment.
Should these be protected by the lock? To prevent the possibility of a double notification of the same snapshot. Not a critical issue, admittedly.
There was a problem hiding this comment.
The event dispatch always runs serially: They run in the single threaded executor.
However locking is necessary to guarantee the following invariant holds:
PendingChangeNotification == trueonly whenissuedSnapshot < pendingSnapshotPendingChangeNotification == falsewhenissuedSnapshot >= pendingSnapshot
Clearly this invariant is not consistent in the event dispatch thread.
I will fix this.
|
|
||
| issuedSnapshot = pendingSnapshot | ||
| watchers.forEach { watcher -> | ||
| watcher.invoke(newSnapshot(pendingSnapshot)) |
There was a problem hiding this comment.
Should probably be notifying with the issuedSnapshot rather than the pendingSnapshot.
There was a problem hiding this comment.
Oh, definitely!
Thanks for spotting this.
| private val pendingChangeNotification = AtomicBoolean(false) | ||
| private val lock = ReentrantLock() | ||
|
|
||
| private val listeners = ConcurrentHashMap<String, DispatchListener<T>>() |
There was a problem hiding this comment.
Are the listeners only for use in testing, or is there another purpose?
There was a problem hiding this comment.
Yes. They are only for testing purposes.
| private val pendingChangeNotification = AtomicBoolean(false) | ||
| private val lock = ReentrantLock() | ||
|
|
||
| private val listeners = ConcurrentHashMap<String, DispatchListener<T>>() |
There was a problem hiding this comment.
This code does not seem clear enough to me. For instance, can't we use more meaningful names than watchers and listeners?
We might need to review the segmentation in functions to see if the sequence of function names help making the intent of the code clearer...
There was a problem hiding this comment.
Hi David. Watcher is the documented term in the object database API.
Listener is just an event hook for the debugging purposes. Perhaps I'll rename it to eventHook? But listener is more common in Java world. Or perhaps I'll clarify this in comments?
Fixes issue #525.