Use ActiveSupport::IsolatedExecutionState to get and set thread or fiber variables depending on the environment

[travisbell]

Upon trying to use Mongoid in a Falcon environment we started getting some very weird errors. Specifically errors that looked like the Mongoid scope for a specific request was incorrect and just very wrong for the request being processed. Upon taking a look at how the Threaded class is used within Mongoid, I believe the reason is because it uses threads to store the state of a bunch of different things throughout the lifecycle of a request.

It appears as though this was assumed to be safe given the fact that (more or less) every server until Falcon used either a single thread (no shared state at all, like Unicorn), or multiple threads (for concurrency and shared state, like Puma). In Falcon of course, this is not the case. All requests are processed in Fibers and there's no mutex around accessing these thread variables.

Now, it turns out in Rails 7.2, there was some work done to address this very problem and there's a new config called ActiveSupport::IsolatedExecutionState.isolation_level which defaults to :thread, but allows us to set :fiber instead.

ActiveSupport::IsolatedExecutionState.isolation_level = :fiber

I don't know what the bigger plan for such a change should be, but at least this change is backwards compatible, always defaulting to :thread, just like it always was.

I've been running this change in production for a week now and all our problems have gone away. Falcon and Mongoid seem like good friends now.

There is the caveat here of being a Rails 7.2 feature, and I am not clear on how you guys handle minimum versions so perhaps there needs to be a ActiveSupport version check. In any case, I'm flagging the issue with a potential fix.

[jamis]

Hey @travisbell, thanks for sharing this!

Looking at the implementation of IsolatedExecutionState, it looks like it uses Thread#[] and Thread#[]= internally when the level is set to :thread. However, we found in #5891 that when we used Thread#[] and Thread#[]= to manage Mongoid's state, values set while one fiber was active would not be visible to values set in another fiber (even though both were in the same thread). We had to use Thread#thread_variable_get and Thread#thread_variable_set instead, which ActiveSupport::IsolatedExecutationState does not use.

It sounds like we'll need to roll our own isolation-state implementation to make this work.

[jamis]

Actually, thinking about this some more.... I worry that switching to a fiber-level isolation state would wreak havoc with our own internal use of fibers as a way to flatten an excessively large recursive stack (when processing child callbacks) -- see #5837. As long as you're not using cascading callbacks and embedded children, you'll probably not encounter this, but I worry that as soon as you do, you'll discover the same sort of errors you ran into before (where the Mongoid query state gets lost in certain situations).

I'm not sure how to resolve this; it might be that Mongoid in its current state cannot be entirely compatible with Falcon. 😢

[travisbell]

Thanks for the reply @jamis, I appreciate it.

It sounds like we'll need to roll our own isolation-state implementation to make this work.

Yes, it does sound like that would be the case. I can probably take a stab at it if we're ok with simply tweaking the existing ActiveSupport implementation. That might be the simplest approach because we know it's a sanctioned Rails approach.

As long as you're not using cascading callbacks and embedded children, you'll probably not encounter this, but I worry that as soon as you do, you'll discover the same sort of errors you ran into before (where the Mongoid query state gets lost in certain situations).

We don't, so you're correct in saying we haven't encountered it yet, but could should we ever need to. For the moment, this is an acceptable caveat but thanks for bringing it up. I know what to look at it if we see the weirdness again.

I'm not sure how to resolve this; it might be that Mongoid in its current state cannot be entirely compatible with Falcon. 😢

😢

[jamis]

I can probably take a stab at it if we're ok with simply tweaking the existing ActiveSupport implementation.

I appreciate that you're willing to dig into this! Tweaking the existing ActiveSupport implementation seems like the most straightforward way to go, but we probably don't even need a separate class for this, since Mongoid already isolates all those accesses in Mongoid::Threaded.get and Mongoid::Threaded.set. We can probably just add the necessary logic there, directly.

That said... I hesitate to go this route at all, simply because if we expose an option for people to select a :fiber isolation level, it implicitly suggests that there should be no problems with doing so. With our current implementation, that's not the case, as I mentioned before. If someone switches to :fiber isolation level and then encounters the issue with cascading callbacks, it will justifiably be viewed as a bug. Fixing that bug, however, is extremely non-trivial. Using fibers was our best attempt so far at addressing it. It's a bit of a catch-22...

[travisbell]

Ok, I'll see if I can find some time to start digging into this over the next few weeks.

Thanks for extra context around #5837. I'll take a look into that as well.

[jamis]

@travisbell -- I've created a ticket in Jira for this, since we definitely want to make this work better as fiber-oriented libraries become more prevalent.

https://jira.mongodb.org/browse/MONGOID-5882

Note the first comment, though -- I describe a possible approach that may wind up working without any radical changes. I intend to try it out at some point, but it might not be soon. You're absolutely not obligated to give it a try yourself, but if it sounds interesting to you, my feelings won't be hurt if you beat me to a solution. :)

In the meantime, I'm going to close this pull request. I mostly just wanted to give you a heads-up that we have a promising lead on how we might fix the problem here.

[travisbell]

Sounds good, thanks for the update!

[jamis]

I lied. I couldn't stop thinking about it and had to give it a try. :) @travisbell -- if you have a chance (and are still needing this functionality) would you mind taking a look at #6009 and seeing if it works for you?

[travisbell]

Amazing. 🤩

I’ll take. Look tomorrow.