Skip to content

[ji] do not expose InterfaceImpl classes in Ruby land#8503

Merged
kares merged 2 commits intojruby:masterfrom
kares:fix-interface-impl_class_set_const
Dec 19, 2024
Merged

[ji] do not expose InterfaceImpl classes in Ruby land#8503
kares merged 2 commits intojruby:masterfrom
kares:fix-interface-impl_class_set_const

Conversation

@kares
Copy link
Member

@kares kares commented Dec 10, 2024

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 InterfaceImpl generated classes synthetic and excluded those from being set in Ruby land...

@kares kares requested a review from headius December 10, 2024 12:37
@kares kares added this to the JRuby 9.4.10.0 milestone Dec 10, 2024
@kares kares marked this pull request as ready for review December 10, 2024 12:40
Copy link
Member

@headius headius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kares kares force-pushed the fix-interface-impl_class_set_const branch from 37c2cab to b6fe7fe Compare December 18, 2024 10:53
@headius
Copy link
Member

headius commented Dec 18, 2024

@kares All good on my end, merge whenever you are ready!

@kares kares merged commit cc58b81 into jruby:master Dec 19, 2024
95 checks passed
@kares kares deleted the fix-interface-impl_class_set_const branch December 19, 2024 13:08
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.
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.

2 participants