Skip to content

Concurrent load interlock (rm Rack::Lock)#17102

Merged
tenderlove merged 4 commits into
rails:masterfrom
matthewd:load-interlock
Jul 10, 2015
Merged

Concurrent load interlock (rm Rack::Lock)#17102
tenderlove merged 4 commits into
rails:masterfrom
matthewd:load-interlock

Conversation

@matthewd

Copy link
Copy Markdown
Member

Instead of forcing Rack::Lock when eager_load and cache_classes aren't on, just prevent actual conflicts.

We achieve this by means of a (reentrant) shared-exclusive lock: multiple running concurrent requests are fine, but any load activity must be performed in isolation.

@thedarkone

Copy link
Copy Markdown
Contributor

Are you aiming for a proper fix to #15089?

Why this might not work:

# foo.rb
class Foo # L1
  # L2
  def self.foo; end # L3
end # L4

Initial state:

  • foo.rb is not loaded,
  • nothing is being auto-loaded,
  • 2 threads get through ActionDispatch::LoadInterlock and enter the "app space".

Both threads attempt to Foo.foo:

Thread A Thread B
does Foo.foo first
obtains interlock and starts loading foo.rb
Executes foo.rb#L1, Foo is now defined
Is executing foo.rb#L2 does Foo.foo, Foo is defined (no need to invoke AS::Dependencies and obtain interlock), but Foo.foo is not, gets a NoMethodError (undefined method 'foo' for Foo:Class).

Thoughts 😞?

Another idea: upon entering Rack::Lock-thingy a thread does eager_load!, once finished app becomes lock-less and threads are let through, when a thread calls AS::Dependencies.clear it is blocked until all other threads leave "app-space" (while all threads wanting to pass through Rack::Lock are blocked), but then what happens when 2 threads are let through and the both call AS::Dependencies.clear?

/cc @fxn.

@matthewd

Copy link
Copy Markdown
Member Author

@thedarkone thread A can't obtain the exclusive lock (and thus start loading foo.rb) while thread B is in "app space": it'll wait for thread B to also try to autoload.

@thedarkone

Copy link
Copy Markdown
Contributor

Alright, then I can't think of a reason why this wouldn't work!

I have maybe 2 suggestions:

  • this is subjective, but your lock AS::Concurrency::ShareLock might be better re-named (including its methods, i-vars etc) as a ReadWriteLock (or ReentrantReadWriteLock), since this what this type of lock is usually known as...
  • a thread trying to obtain an exclusive (write) lock should block any new (non-reentrant) readers/sharers, otherwise it is prone to starvation.

btw: I'm seriously impressed, I never thought making AS::Dep thread-safe was realistically possible 😱, this PR is blowing my mind, big kudos to @matthewd 😎!

@fxn

fxn commented Oct 1, 2014

Copy link
Copy Markdown
Member

+1 on being impressed. If we do not discover any devil in the details, this is going to be a real win.

@rafaelfranca

Copy link
Copy Markdown
Member

@matthewd are you thinking in moving this forward? I believe this is the best time to do it.

@fxn

fxn commented Feb 6, 2015

Copy link
Copy Markdown
Member

👍 makes sense for 5.0.

@thedarkone

Copy link
Copy Markdown
Contributor

I'm thinking - this will not work if user decides to spin up his own threads (because we will not be able to track whether they have entered "app space"). Also, how would this work with background jobs?

@matthewd

Copy link
Copy Markdown
Member Author

For background jobs: the job-runner would wrap the job-doing code just as ActionDispatch does for requests. (And as it presumably already does for the AR conn pool, for example.)

If the user spins up their own thread (and doesn't opt in to the interlock), however... yeah, they're going to be in danger of seeing a half-loaded constant. Avoiding that sounds expensive. 😕

We don't need to fully disable concurrent requests: just ensure that
loads are performed in isolation.
@matthewd matthewd closed this Jul 8, 2015
@matthewd matthewd reopened this Jul 8, 2015
matthewd added 2 commits July 9, 2015 03:31
We can't actually lean on Rack::Lock's implementation, so we'll just
copy it instead. It's simple enough that that's not too troubling.
tenderlove added a commit that referenced this pull request Jul 10, 2015
Concurrent load interlock (rm Rack::Lock)
@tenderlove tenderlove merged commit 8f81f7a into rails:master Jul 10, 2015
@mperham

mperham commented Jul 17, 2015

Copy link
Copy Markdown
Contributor

This is so awesome. I think we can modify Sidekiq to reload job code in development mode now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants