Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make LoadInterlockAwareMonitor work in Ruby 2.7 #38069

Merged
merged 1 commit into from Dec 23, 2019
Merged

Make LoadInterlockAwareMonitor work in Ruby 2.7 #38069

merged 1 commit into from Dec 23, 2019

Conversation

y-yagi
Copy link
Member

@y-yagi y-yagi commented Dec 22, 2019

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.

@y-yagi y-yagi requested a review from matthewd December 22, 2019 05:05
kamipo
kamipo reviewed Dec 22, 2019
View reviewed changes
activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb Outdated
# Enters an exclusive section, but allows dependency loading while blocked
def mon_enter
mon_try_enter ||
ActiveSupport::Dependencies.interlock.permit_concurrent_loads { super }
end

def synchronize
Thread.handle_interrupt(Exception => :never) { mon_enter }
Copy link
Member

Choose a reason for hiding this comment

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

How about extract Exception => :never as constant?
Now each synchronize call allocate new Exception => :never hash twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your review. I fixed.

@y-yagi y-yagi force-pushed the make_load_interlock_aware_monitor_work_in_ruby27 branch from 1a61dd8 to 3c1437c Compare December 22, 2019 07:13
matthewd
matthewd reviewed Dec 22, 2019
View reviewed changes
Copy link
Member

@matthewd matthewd left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this!

I agree this is the best fix 👍🏻

activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb Outdated
@@ -6,12 +6,24 @@ module ActiveSupport
module Concurrency
# A monitor that will permit dependency loading while blocked waiting for
# the lock.
class LoadInterlockAwareMonitor < Monitor
class LoadInterlockAwareMonitor < Monitor # :nodoc:
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry I misunderstood this is a private API. I will fix to keep this public.

Copy link
Member

Choose a reason for hiding this comment

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

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.

activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb Outdated
begin
yield
ensure
Thread.handle_interrupt(EXCEPTION_NEVER) { mon_exit }
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ;)

@y-yagi
Make LoadInterlockAwareMonitor work in Ruby 2.7
4f4f8a7 
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
@y-yagi y-yagi force-pushed the make_load_interlock_aware_monitor_work_in_ruby27 branch from 3c1437c to 4f4f8a7 Compare December 22, 2019 22:26
@y-yagi y-yagi merged commit a68f114 into rails:master Dec 23, 2019
@y-yagi y-yagi deleted the make_load_interlock_aware_monitor_work_in_ruby27 branch December 23, 2019 23:18
y-yagi added a commit that referenced this pull request Dec 24, 2019
@y-yagi
Merge pull request #38069 from y-yagi/make_load_interlock_aware_monit…
b14f56f 
…or_work_in_ruby27

Make `LoadInterlockAwareMonitor` work in Ruby 2.7
@yahonda yahonda mentioned this pull request Dec 24, 2019
Closed
eileencodes pushed a commit that referenced this pull request Apr 13, 2022
@y-yagi @eileencodes
Merge pull request #38069 from y-yagi/make_load_interlock_aware_monit…
de20f93 
…or_work_in_ruby27

Make `LoadInterlockAwareMonitor` work in Ruby 2.7
@eileencodes eileencodes mentioned this pull request Apr 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewd matthewd matthewd left review comments

@kamipo kamipo kamipo left review comments

Assignees
No one assigned
Labels
activesupport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@y-yagi @matthewd @kamipo