[ji] do not expose InterfaceImpl classes in Ruby land#8503
Merged
kares merged 2 commits intojruby:masterfrom Dec 19, 2024
Merged
[ji] do not expose InterfaceImpl classes in Ruby land#8503kares merged 2 commits intojruby:masterfrom
kares merged 2 commits intojruby:masterfrom
Conversation
headius
approved these changes
Dec 12, 2024
Member
headius
left a comment
There was a problem hiding this comment.
This is genius in its simplicity... of course we wouldn't want to bind synthetic classes, and hidden classes we generate should obviously be marked as synthetic.
37c2cab to
b6fe7fe
Compare
Member
|
@kares All good on my end, merge whenever you are ready! |
headius
added a commit
to headius/jruby
that referenced
this pull request
May 20, 2025
Synthetic classes, such as our own "InterfaceImpl" proxies generated on-the-fly to adapt Ruby objects to Java interfaces, are generally not supposed to be visible to users. A change in JRuby 9.4 to reduce wrapping overhead for InterfaceImpl proxy objects led to them also being bound as Ruby proxy classes, which had the effect of causing these frequently-generated and frequently- abandoned classes to be pinned in memory by the proxy class's method bindings. This led to a class and metaspace leak reported in jruby#8842. The fix here explicitly avoids binding any synthetic classes, instead skipping such classes and walking up to the nearest non- synthetic class before beginning the binding process. This avoids our InterfaceImpl classes from being bound as Ruby proxies and appears to eliminiate at least the leak reproduced in jruby#8842. This may also fix other undiscovered or un-reproduced cases of generated classes leaking into the Ruby proxy class hierarchy, so long as all of those classes are being generated as synthetic. This may also be a more direct and appropriate fix for the eager binding and logging of InterfaceImpl classes reported in jruby#8349, which was previously fixed by avoiding synthetic classes when adding proxies to our Java package namespace structures (jruby#8503). If that is the case, we may want to consider turning that check into an error, since it will probably always be incorrect to have synthetic classes get bound into a public namespace. Additional checks elsewhere in our Java Integration subsystem could also help prevent similar issues in the future, and we should audit all generated classes to ensure they are marked as synthetic. Fixes jruby#8842. May be a better fix for jruby#8349 than jruby#8503 but that fix's check should perhaps remain or become a hard error. Needs tests for as many JI class generation cases as possible.
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
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.
Yet another attempt at resolving #8349, for good.
Since the reverts done in JRuby 9.4.9.0 still generate the warnings.
Seems like there's more (related) stuff that changed (since 9.4.6.0) given the checks, changed in this PR, were there but did not cause warnings 🤷.
Instead of chasing around I took an approach of marking
InterfaceImplgenerated classes synthetic and excluded those from being set in Ruby land...