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

Merged
merged 4 commits into from Jul 10, 2015

Projects

None yet

6 participants

@matthewd
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
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
Member

@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
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
Member
fxn commented Oct 1, 2014

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

@rafaelfranca
Member

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

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Feb 6, 2015
@fxn
Member
fxn commented Feb 6, 2015

👍 makes sense for 5.0.

@thedarkone
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
Member

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

@matthewd matthewd Soften the lock requirements when eager_load is disabled
We don't need to fully disable concurrent requests: just ensure that
loads are performed in isolation.
c37d47e
@matthewd matthewd closed this Jul 8, 2015
@matthewd matthewd reopened this Jul 8, 2015
matthewd added some commits Sep 29, 2014
@matthewd matthewd Rely on the load interlock for non-caching reloads, too 383fed5
@matthewd matthewd Fix the Interlock middleware
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.
48a735a
@matthewd matthewd Document ShareLock and the Interlock 0b93c48
@tenderlove tenderlove merged commit 8f81f7a into rails:master Jul 10, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mperham
Contributor
mperham commented Jul 17, 2015

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

@matthewd matthewd deleted the matthewd:load-interlock branch Aug 22, 2015
@rafaelfranca rafaelfranca modified the milestone: 5.0.0 [temp], 5.0.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment