Eager load the constants #102
Conversation
|
r? @kaspth (@rails-bot has picked a reviewer for you, use r? to override) |
y-yagi
force-pushed
the
eager_load_constants
branch
from
f78e9b1
to
9df399e
Compare
There was a problem hiding this comment.
The API docs say we don't need to add the file paths. Will it work without them here?
http://api.rubyonrails.org/classes/ActiveSupport/Autoload.html#method-i-eager_autoload
| @@ -1,9 +1,14 @@ | |||
| require 'global_id/global_id' | |||
| require 'active_support' | |||
|
|
|||
| autoload :SignedGlobalID, 'global_id/signed_global_id' | |||
There was a problem hiding this comment.
I think we also need to eager load this.
There was a problem hiding this comment.
Agree.
But SignedGlobalID is defined at Top level. Is eager load usable in that case?
There was a problem hiding this comment.
Not really. We can implement self.eager_load! in GlobalID and make it require that file.
There was a problem hiding this comment.
Understood, thank you! I fixed.
|
We also need to add GlobalID to the |
|
This is a rather complicated work-around for a Ruby bug that no-one's managed to reproduce. 😢 If people who've experienced the problem (and therefore obviously have a setup that's theoretically capable of reproducing) could help track down the upstream bug, we could stop chasing autoloads through every gem. |
|
It seems like a workaround for that bug it is rather a way to improve CoW mechanism of the gem. Instead of loading that constant in every forked process we do load it only in the parent process. Regardless of that bug I think this should be implemented. |
y-yagi
force-pushed
the
eager_load_constants
branch
from
9df399e
to
a70d5cf
Compare
Looks it works without them. I removed files paths. |
In a thread-based backend like Sidekiq, there is a possibility that autoload may occur simultaneously in multiple threads, and as a result, it is presumed that an error may be caused by contention of autoload. In order to avoid the above issue, eager load the constants on boot. Maybe fixes rails#101
y-yagi
force-pushed
the
eager_load_constants
branch
from
a70d5cf
to
797a383
Compare
|
Are you planning to release an update to the gem with these changes? |
|
Yes. 0.4.1 was just released |
In a thread-based backend like Sidekiq, there is a possibility that autoload may occur simultaneously in multiple threads, and as a result, it is presumed that an error may be caused by contention of autoload.
In order to avoid the above issue, eager load the constants on boot.
Maybe fixes #101
Context: rails/rails#30217 (comment)