Fix autoloading issue with ActiveStorage::Downloader #34253
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
|
The funny thing is that classes in the framework are not autoloaded like classes in the application EDIT: This remark is incorrect. Part of Active Storage does use Rails autoloading because it is an engine with models and other autoloadable stuff. Could you come with a minimal example to reproduce? |
|
Didn't know how to come up with something really minimal like a small script, but i created a minimal demo app here: https://github.com/haberbyte/activestorage_downloader_error Steps to reproduce:
=> Now i see the error whenever a view tries to get the image. Stops only after killing and restarting the server. I hope you can follow the steps and make more sense of this than me :-D Here is a full example stacktrace: |
|
@haberbyte perfect mini application, could reproduce. |
|
I believe this is a bug in dependencies. Let me explain. That The problem I notice is that although that constant is not autoloaded, dependencies believes the constant has been autoloaded. You can throw before_action -> { logger.debug(ActiveSupport::Dependencies.autoloaded_constants) }in The bug is that dependencies believes the constant was autoloaded, but it wasn't. |
|
Oh wow, thanks for that explanation, makes sense. I am struggling to understand how ActiveSupport::Dependencies works. When Dependencies loads a file: # lib/active_support/dependencies.rb
def load_file(path, const_paths = loadable_constants_for_path(path))
const_paths = [const_paths].compact unless const_paths.is_a? Array
parent_paths = const_paths.collect { |const_path| const_path[/.*(?=::)/] || ::Object }
result = nil
newly_defined_paths = new_constants_in(*parent_paths) do
result = Kernel.load path
end
autoloaded_constants.concat newly_defined_paths unless load_once_path?(path)
autoloaded_constants.uniq!
result
endKernel.load will load "/activestorage/app/models/active_storage/blob.rb", which requires stuff from lib/ and therefore adds a new constant to ActiveStorage. Newly defined paths now gives: But Downloader (and actually LogSubscriber as well) is in a path outside of the autoload paths, so this should be filtered out before adding it to |
|
Exactly, good analysis. This works well routinely, for example, you can Without looking at the code, I suspect the watch stack manager or someone involved in deciding what has been autoloaded might be incorrectly assuming something about being in a different namespace. |
|
I have reproduced with an even smaller application that doesn't use Active Storage. There, we have app/models/admin/user.rb, which requires lib/admin/utils.rb. The former is autoloadable, the latter is not. To demonstrate the problem we have foo.rb in the root directory: Admin::User
p ActiveSupport::Dependencies.autoloaded_constantsthat you need to execute with
|
|
Just realized, though, that Perhaps that is saying this is an inherent limitation of Rails autoload more than a bug. In which case, the approach to solve it could be different, like documenting it, rewriting Active Storage directories, etc. |
|
I just realized that even things like the AS errors, e.g. With rewriting directories, do you mean having a different module name in lib than in app? Just thinking out loud, but couldn't rails autoloader be extended to keep track of what constants have been added by an autoload, but are not part of an autoload path? Wow this gets complicated ;-) |
Is this true for
Would |
|
@fxn dude why me? |
Bad completion selection, sorry! I have deleted the comment. |
|
@georgeclaghorn Active Storage is indeed not affected by that gotcha with the root namespace and it is doing things correctly, because the There it is, I believe |
|
The docs on top of the ...
# If child.rb is being autoloaded, its constants will be added to
# autoloaded_constants. If it was being required, they will be discarded.Well, the discard is not really working in this case. And I don't see how this should work. When the WatchStack is called to watch the "Admin" namespace, it keeps track of the original constants, so that it can know which ones are new. (In this case, original constants is empty since Admin is an empty module so far). But |
|
@fxn I quite honestly have little idea what I am doing, but after a long run of puts-debugging I pushed a possible fix. If you try your demo-app with this branch of rails, then you should get: |
|
@haberbyte Bravo! Digging into dependencies.rb demonstrates courage 😄. I need to do some work to validate the patch, but my first impression is that it could be correct. Would you like to provide test coverage? |
|
I can try to add a test for this, might take a me a while, but I guess there are enough other tests to take a look how it should be done ;) |
|
@fxn I added a test that fails without my fix and passes with it. Hope the implementation is somehow ok, happy to improve as needed. |
|
Awesome @haberbyte. I'll write back! |
|
Hey! I believe the patch is good, only don't quite see a way in which Dependencies.constant_watch_stack.watching.lastcould be Dependencies.constant_watch_stack.watching?which means that Did you add |
|
Yes. I added that just to play safe and since it was used before this patch by default. |
|
OK, I believe it is not needed. Could you please remove it? In the case I missed something I prefer to be proved wrong with a regression and understand why. |
|
Nothing like sleeping on a problem. Unfortunately, I have found a simple case that is broken by this change: Now: As you see, The problem is that when you are autoloading OK, this matches my intuition above: there is code assuming things about namespaces. My hypothesis is that when I believe the correct patch is That is, ignore anything loaded in any of the watched namespaces. |
|
Ah wow, nice catch. Should I try to add your example as another test case and try your suggested patch with it? I still need to take some time to truely understand this, my first patch attempt was just my best guess. Thanks for taking the time to look into this, I'm learning alot :-) |
|
@haberbyte that would be great, let's add yours and mine use cases to the test suite, and let's merge. Not only is autoloading tricky and difficult to follow when you go pass the golden path and get into the details the actual implementation has to account for, but in addition So congrats on digging into it, the original patch was maybe not the final solution, but you identified the suspicious spot, which is awesome. So congrats on the effort ❤️. |
|
OK i again added a failing test and made sure it fails with the current implementation, and is fixed by the change you suggested. I find it incredibly hard to even name the test-case. |
|
The commit message could be something like this: What do you think? |
|
Oh, didn't say it before, but I guess we could rebase, and leave one single commit with (for example) that commit message. |
|
Yup, will do that once I’m back home. I like that message, thank you! |
If you require `nokogiri` from `app/models/user.rb`, dependencies.rb does not mark `Nokogiri` as an autoloaded constant, as expected. But the logic to detect these non-autoloaded constants is incomplete. See the tests defined in the patch for some cases incorrectly handled.
haberbyte
force-pushed
the
fix_autoload_on_activestorage_downloader
branch
from
2543676
to
e302725
Compare
|
Rebased and just used your message. I am wondering if the sentence is true though, does it really work for nokogiri, or is that just because nokogiri tends to be required before autoloading kicks in? |
|
It has worked for Nokogiri since forever (Nokogiri being just an example of a 3rd party file).
But the examples we’ve seen say the support was not sufficient. The message wants to explain which is the feature we are talking about. I am going to merge, later at home I’ll add an entry in the CHANGELOG and backport to 5.2. |
|
@fxn is this safe to backport? |
|
I think so, the logic is the same as the one in I backported because anyone using Active Storage with 5.2 may be affected by this bug (that was the issue that originated this fix). |
|
Ok. I didn't see the backport commit here so this is why I asked. I Agree we should backport. |
There is currently an issue in local development that cause the following error:
This seems to happen whenever I change some models in my app that make use of ActiveStorage and try to download a new representation of a file.
Not sure, maybe it is related to this gotcha: https://edgeguides.rubyonrails.org/autoloading_and_reloading_constants.html#when-constants-aren-t-missed
In any case, I looked at where the ActiveStorage::Downloader class is used, and it is only used in "app/models/active_storage/blob.rb". Since the usage is in app/, moving it also into "app/models/" made sense to me.
This fixed the issue for me, and the Blob model now does not need to manually require "active_storage/downloader" anymore.