-
-
Notifications
You must be signed in to change notification settings - Fork 69
Track and unregister loaders in tests manually #293
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
base: main
Are you sure you want to change the base?
Conversation
Previously I enabled ObjectSpace in JRuby in zeitwerk_helpers.rb programatically, but it did not fully work. ObjectSpace.each_object(Zeitwerk::Loader) kept returning empty list. I suppose this was because the ObjectSpace was enabled too late. It worked correctly when JRUBY_OPTS="-X+O" wever passed in the command line. However, because the CI config is set via repo automation, it was hard to configure it just for dry-systems and would have a performance penalty if we enabled it for all gems. So instead, I figured out a way to drop usage of ObjectSpace in favour of Zeitwer's Registry module, which does the same thing (presumably even better).
f091fc2 to
c0e41f5
Compare
|
Thank you for persisting with this, @katafrakt! We actually did use See #259 and #260 for that discussion. The last thing I want is to make @fxn unhappy (😆), so if ObjectSpace is not easy for us to use under JRuby, I wonder if there's a third option we could take here? Is there a way we could track our own loaders more clearly, e.g. our own version of |
|
That is the idea. Zeitwerk does not have interface to fetch loaders because loaders are supposed to be private to gems, I cannot expose and bless accessing them. But a project that needs to know about its loaders can have its own registry. |
This replaces ZeitwerkHelpers with ZeitwerkLoaderRegistry, which keeps track of created loaders and allows to unregister them. This is used in tests when we need to have multiple loaders but cannot allow them to interfere with each other. This replaces former solutions to the problem: - Using Zeitwerk::Loaders - it's a private API, not guaranteed to persist - Using ObjectSpace - this does not work well in JRuby
timriley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @katafrakt! Looks like we're heading in the right direction now. I did have one question for you, though. It seems like we may be missing the cleanup of the default loaders created by the plugin, for the times when we're not passing them in explicitly?
| include ZeitwerkHelpers | ||
|
|
||
| after { teardown_zeitwerk } | ||
| after { ZeitwerkLoaderRegistry.clear } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the loaders created in this example group get torn down? Here we're letting the plugin create the loader itself (which is just a plain old ::Zeitwerk::Loader.new), rather than assigning a loader that we've created ourselves via ZeitwerkLoaderRegistry.new_loader.
(This question applies to any of the spec files that follow a similar approach to this one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point. One thing to remedy this could be to just stub Zeitwerk::Loader.new in tests with ZeitwerkLoaderRegistry.new_loader, but I'm not sure how we feel about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to remedy this could be to just stub
Zeitwerk::Loader.newin tests withZeitwerkLoaderRegistry.new_loader, but I'm not sure how we feel about this.
I think this is no worse than what we were doing to iterate over ObjectSpace and find the classes we want. So I think it's a reasonable move at this point just to keep things us moving forward and unlock JRuby support.
We should at least file an issue about finding a cleaner way of doing this.
Changes in #292 turned out to not be enough. While they removed the error of
ObjectSpacenot being enabled, it still did not work correctly, presumably because it was enabled too late. As a result,ObjectSpace.each_object(Zeitwerk::Loader)was yielding empty set and the loader were not unregistered successfully, leading to errors like this:This replaces
ZeitwerkHelperswithZeitwerkLoaderRegistry, which keeps track of created loaders and allows to unregister them. This is used in tests when we need to have multiple loaders but cannot allow them to interfere with each other.This replaces former solutions to the problem:
Zeitwerk::Loaders- it's a private API, not guaranteed topersist
ObjectSpace- this does not work well in JRuby