Repository navigation
Make LoadInterlockAwareMonitor work in Ruby 2.7 - #38069
Conversation
There was a problem hiding this comment.
How about extract Exception => :never as constant?
Now each synchronize call allocate new Exception => :never hash twice.
There was a problem hiding this comment.
Thanks for your review. I fixed.
1a61dd8 to
3c1437c
Compare
matthewd
left a comment
matthewd
left a comment
There was a problem hiding this comment.
Thanks for looking at this!
I agree this is the best fix 👍🏻
There was a problem hiding this comment.
Oh, sorry I misunderstood this is a private API. I will fix to keep this public.
There was a problem hiding this comment.
It's not "major" public API, but it's a useful tool for external users who need to lock inside a possibly-reloading Rails process -- and the call signatures aren't likely to change, so it seems fine to me for it to be exposed.
There was a problem hiding this comment.
I don't think this is safe: an interrupt could occur just before calling Thread.handle_interrupt, unwinding this method without ever calling mon_exit
There was a problem hiding this comment.
Hmm, I see. So what do you think of the following fix?
--- a/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb
+++ b/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb
@@ -6,9 +6,10 @@ module ActiveSupport
module Concurrency
# A monitor that will permit dependency loading while blocked waiting for
# the lock.
- class LoadInterlockAwareMonitor < Monitor # :nodoc:
+ class LoadInterlockAwareMonitor < Monitor
EXCEPTION_NEVER = { Exception => :never }.freeze
- private_constant :EXCEPTION_NEVER
+ EXCEPTION_IMMEDIATE = { Exception => :immediate }.freeze
+ private_constant :EXCEPTION_NEVER, :EXCEPTION_IMMEDIATE
# Enters an exclusive section, but allows dependency loading while blocked
def mon_enter
@@ -17,11 +18,16 @@ def mon_enter
end
def synchronize
- Thread.handle_interrupt(EXCEPTION_NEVER) { mon_enter }
- begin
- yield
- ensure
- Thread.handle_interrupt(EXCEPTION_NEVER) { mon_exit }
+ Thread.handle_interrupt(EXCEPTION_NEVER) do
+ mon_enter
+
+ begin
+ Thread.handle_interrupt(EXCEPTION_IMMEDIATE) do
+ yield
+ end
+ ensure
+ mon_exit
+ end There was a problem hiding this comment.
Yeah, that looks right to me.
Unfortunate it looks so ugly, and pollutes the call stack, but I think it's the only way to implement it safely. I wish there was built-in syntax to make this cleaner (and harder to get wrong).
There was a problem hiding this comment.
Thanks for your review! I fixed.
I wish there was built-in syntax to make this cleaner
Agree. We should create a proposal to bugs.ruby-lang ;)
Currently `LoadInterlockAwareMonitorTest` does not pass with Ruby 2.7 [1]. This is due to the refactoring of the `monitor` done in Ruby 2.7 [2]. With this refactoring, the behavior of the method has changed from the expected behavior in `LoadInterlockAwareMonitor`. This patch also overwrites `synchronize` so that `LoadInterlockAwareMonitor` works as expected. [1]: https://buildkite.com/rails/rails/builds/65877#eec47af5-7595-47cb-97c0-30c589716176/996-2743 [2]: https://bugs.ruby-lang.org/issues/16255
3c1437c to
4f4f8a7
Compare
…or_work_in_ruby27 Make `LoadInterlockAwareMonitor` work in Ruby 2.7
…or_work_in_ruby27 Make `LoadInterlockAwareMonitor` work in Ruby 2.7
Currently
LoadInterlockAwareMonitorTestdoes not pass with Ruby 2.7 1.This is due to the refactoring of the
monitordone in Ruby 2.7 2.With this refactoring, the behavior of the method has changed from the expected behavior in
LoadInterlockAwareMonitor.This patch also overwrites
synchronizeso thatLoadInterlockAwareMonitorworks as expected.