Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sync and profiling method wrapper logic work with refinements #6549

Merged
merged 1 commit into from Feb 12, 2021

Conversation

headius
Copy link
Member

@headius headius commented Feb 3, 2021

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.

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.
@headius headius added this to the JRuby 9.2.15.0 milestone Feb 3, 2021
@headius
Copy link
Member Author

headius commented Feb 12, 2021

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.

@headius headius merged commit b3c9fa9 into jruby:jruby-9.2 Feb 12, 2021
@headius headius deleted the fix_synchronized_refinement branch February 12, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant