Add RW locking to better manage concurrency #8997
Conversation
Signed-off-by: Tim Quinn <[email protected]>
oracle-contributor-agreement
bot
added
the
OCA Verified
| lock.readLock().lock(); | ||
| List<io.helidon.metrics.api.Meter> temp; | ||
| try { | ||
| temp = new ArrayList<>(meters.values()); |
There was a problem hiding this comment.
Hmm; this idiom with the streaming below it seems verbose. What about:
return List.copyOf(meters.values()).stream()
.map(...)
....
…all here under lock instead?
There was a problem hiding this comment.
Copying the array inside the lock but doing the rest of the work outside lets us release the lock sooner. Maybe not a huge difference but we might as well hold the lock for as short a time as we can.
| return scopeMembership.keySet(); | ||
| lock.readLock().lock(); | ||
| try { | ||
| return new HashSet<>(scopeMembership.keySet()); |
There was a problem hiding this comment.
Is it OK if someone casts the returned Iterable to a Set and starts adding things to it? If not, consider:
return Set.copyOf(scopeMembership.keySet());
There was a problem hiding this comment.
Yes, it's OK as far as I'm concerned.
We've copied the keys into a new set that is not backed by the original map, so the calling code can do what it wants with the returned set without disturbing our internal set.
Or did I miss your point?
There was a problem hiding this comment.
I just meant: what are the semantics of the return value? I tend to prefer: "the return value is immutable". You're saying, I think: "the return value's mutability is undefined". Either can be true; just wanted clarification.
| } | ||
| return result; |
There was a problem hiding this comment.
As above, we create a new result set that is not backed by our internal set, so I'm not sure why we'd restrict what the caller can do with the return value.
| try { | ||
| onAddListeners.forEach(listener -> listener.accept(result)); | ||
| } catch (Exception e) { | ||
| // Ignored. |
There was a problem hiding this comment.
Ding ding ding. My alarm bells go off; worthy of a log at least I would think
There was a problem hiding this comment.
[I responded to this comment earlier but maybe I didn't save it.]
I've removed the try/catch block. Everywhere else where we invoke the listeners we (intentionally) let any exceptions bubble up. We should do the same here.
| } | ||
|
|
||
| lock.lock(); | ||
| /* | ||
| "Promote" our lock from read to write. |
There was a problem hiding this comment.
…although you don't have any lock at this point?
There was a problem hiding this comment.
Well, not at that moment--hence the quotes around "promote" because in fact just a few lines above we had acquired the read lock and the effect we want is to promote the lock from read to write as the comment a few lines below explains.
I've rewritten the comments to remove any possible confusion.
There was a problem hiding this comment.
LGTM. Acquiring WL and then checking condition again to account for the interim no lock was held.
* Add RW locking to better manage concurrency Signed-off-by: Tim Quinn <[email protected]> * Address review comments --------- Signed-off-by: Tim Quinn <[email protected]>
* Add RW locking to better manage concurrency --------- Signed-off-by: Tim Quinn <[email protected]> Co-authored-by: Tim Quinn <[email protected]>
Description
Resolves #8980
The Helidon metrics implementation which uses Micrometer involves several interrelated data structures. Updates to these have always been protected by a lock, but the referenced issue reveals an oversight: read-only operations (which did not participate in locking) could fail in odd ways because the backing data structure for a returned list or set could change.
This PR enhances the locking scheme to use a
ReadWriteReentrantLock.Notable points:
getOrCreate, starts with a read lock and "promotes" to the write lock as needed. Of courseRWRLdoes not itself support that but the changes togetOrCreatedo. See the comments in the revision to the code for details.Documentation
Bug fix; no doc update.