Make sync and profiling method wrapper logic work with refinements#6549
Merged
headius merged 1 commit intojruby:jruby-9.2from Feb 12, 2021
Merged
Make sync and profiling method wrapper logic work with refinements#6549headius merged 1 commit intojruby:jruby-9.2from
headius merged 1 commit intojruby:jruby-9.2from
Conversation
Refinements installed over the top of an existing method get readded to the method table with a RefinedWrapper dynamic method wrapper. When this is combined with our Synchronized module, it gets wrapped again with a SynchronizedDynamicMethod that the refinment logic is unaware of. As a result, the eventual retrieval of the inner wrapped method does not happen and methods that are clearly defined trigger method_missing. The fix here detects the delegated wrapper and unwraps it before proceeding to the rest of the refinement logic, restoring the proper behavior. This is only a partial fix, since there are other places where the wrapping can interfere with method lookup logic. It partially addresses jruby#6547 but a more complete fix is still needed.
Member
Author
|
The larger fixes to change how Synchronized and profiling works will need to come in a separate PR, and probably only applied to master due to the invasiveness of the change. |
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.
This PR will track work to make our two types of "transparent" delegating method wrappers compatible with refinements.
As described in #6547, the wrapper methods that the Synchronized module uses interfere with refinement logic due to the latter's dependence on its own wrapper. When the two are active at the same time, the method lookup process may fail to locate methods that are clearly there, due to not knowing how to access the original unwrapped method properly.
The first fix here is a one-off that pre-unwraps all DelegatingDynamicMethod wrappers before proceeding to the rest of the refined logic, but the issue is systemic; the synchronized wrappers cannot be perfectly transparent, and so they interfere with method table logic that depends on seeing the original method. An alternative solution is needed.
One such solution might be to move the synchronization logic to a specialized CacheEntry, that will provide the wrapper only when being used to invoke. All other accesses would continue to see the original method. This would avoid eagerly wrapping the retrieved method and hiding its true nature, but it would open up the possibility that the unsynchronized version would leak out and be invokable without synchronization further down the line.
The TruffleRuby equivalent to this eagerly replaces all methods with new methods that actively synchronize; they are normal methods, not wrappers, so refinements do not interfere with them. However this only affects methods defined on the class at the point at which the class is flipped to being synchronized; new methods will not be synchronized unless the switch is again flipped. This does not seem to be an acceptable alternative to adding synchronization at lookup or invocation time.