Lock on the current Fiber rather than current Thread

[casperisfine]

Applications using fiber are able to do concurrent queries from the same thread.

Ref: rails/rails#46519 (comment)

NB: I'm not sure when rb_fiber_current() was introduced, so I'm waiting for CI to see.

[casperisfine]

NB: I'm not sure when rb_fiber_current() was introduced, so I'm waiting for CI to see.

Looks like 2.1 has it, no idea for 2.0, but it's not ran on CI.

[casperisfine]

no idea for 2.0

I checked, it has it, I'll remove the conditional.

[ioquatix]

This looks reasonable to me but would it be better to just use a mutex?

[sodabrew]

This is great! I think it may solve quite a few mystery reports of out of sequence commands despite not crossing threads.

Re: mutex, the gem assumes the client connection pool is handling this. Is that assumption worth revisiting?

[sodabrew]

It sounds like there's no need to raise the minimum Ruby for this import, but I definitely would be comfortable raising the baseline for this PR if needed.

[casperisfine]

Yeah I don't think it's needed.

IMHO 2.0 is really really old and people who are still on it don't deserve time. I now require 2.5 across all my projects. But that is totally independent of this PR.

[ioquatix]

Well, it sounds like what you basically have is a poor version of a mutex, since you are tracking a critical section with an active_whatever. If you just used Mutex for this, you wouldn't have this PR, because it would already just work. You can bail on locking a mutex by checking Mutex#locked? or if you want reentrancy, Mutex#owned? (the current execution context has locked it). This is more compatible since not all platforms currently have fiber-level mutex (but there is nothing wrong with the current PR).

[casperisfine]

but would it be better to just use a mutex?

This is essentially the same thing, Ruby's mutex is also just a struct with the current fiber store in. This is essentially an inlined mutex.

We could take it out and use a Mutex on the Ruby side, but that would reduce perf a bit, and expose us to bugs with interrupts and such (we'd need to use Thead.handle_interrupt).

So I'd say no.

I think it may solve quite a few mystery reports of out of sequence commands despite not crossing threads.

Actually I doubt so. Short of people using a fiber scheduler and some weird enumerator usages this shouldn't change much.

However I do think the current protection has a few holes, in several cases we release the lock too early which I think open some small race-condidtion windows that can cause segfaults.

I started working on improving this a bit, but haven't finished yet, I'll try to get back on it: Shopify@13b38da

Re: mutex, the gem assumes the client connection pool is handling this. Is that assumption worth revisiting?

IMO no. You shouldn't use the connection concurrently, you should use a pool as you say. This check is just here so that if you mess it up we raise and error instead of segfaulting.

[casperisfine]

@sodabrew any changes required for this PR to be mergeable?

[ioquatix]

Nice work team.