Ensure subclass set prevents concurrent modification#8603
Merged
headius merged 3 commits intojruby:masterfrom Feb 5, 2025
Merged
Ensure subclass set prevents concurrent modification#8603headius merged 3 commits intojruby:masterfrom
headius merged 3 commits intojruby:masterfrom
Conversation
The new SubclassesWeakMap properly included read/write locking for the `put` and `forEach` methods, but unfortunately the `remove` and `isEmpty` methods were not overridden with locking. This went unnoticed until after the 9.4.11.0 release, leading to jruby#8602. This patch makes two modifications to ensure proper locking: * Instead of working with the full Map interface, and possibly missing other method uses in the future, this narrows the subclasses map interface to a RubyClassSet with only unique methods that we use. This guarantees only these methods can be called, and we can ensure they are implemented properly. * Add the missing locking to the `remove` method, now named `removeClass`. A subsequent commit will include a test of high-concurrency reading and writing to these maps which fails prior to this commit and passes afterwards. Fixes jruby#8602.
This should also test included hierarchy walking and manipulation, which utilizes the same data structure. See jruby#8602
Member
Author
|
An additional change may be needed here due to several read operations mutating the collection to clean out stale entries. We may need to return to a fully locked implementation while using this implementation of WeakHashMap. |
Because this is a weak set, it may mutate itself during read operations to clean up dead references. A read lock is not sufficient to guarantee the collection's integrity under these circumstances, so I replace the read/write locking with a standard ReentrantLock for all supported operations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The new SubclassesWeakMap properly included read/write locking for the
putandforEachmethods, but unfortunately theremoveandisEmptymethods were not overridden with locking. This went unnoticed until after the 9.4.11.0 release, leading to #8602.This patch makes two modifications to ensure proper locking:
removemethod, now namedremoveClass.Fixes #8602.