open-feature / ruby-sdk

Ruby implementation of the OpenFeature SDK
https://openfeature.dev
Apache License 2.0
21 stars 7 forks source link

fix: synchronize provider registration #136

Closed mschoenlaub closed 3 months ago

mschoenlaub commented 3 months ago

This PR

Notes

This likely does not do enough to really make it "threadsafe". It's good enough for the obvious case i stumbled across and should be fine for MRI support.

Follow-up Tasks

Define whether at this stage we want to support "truly parallel" ruby implementations.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.54%. Comparing base (51155a7) to head (f0c1cfb).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #136 +/- ## ======================================= Coverage 99.53% 99.54% ======================================= Files 16 16 Lines 214 218 +4 ======================================= + Hits 213 217 +4 Misses 1 1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mschoenlaub commented 3 months ago

I'm curious if there was a reproduction case you encountered while using the SDK that impacted thread safety, or if this is an anticipated issue with setting providers on the singleton (I can see the reason why multiple threads would potentially overlap here).

Unfortunately, there's still a bit of stuff to do before we can think of actually using this in production. So it's not a full repro case.

I do think that a very basic usage in a, let's say rails application, would work out of the box. You'd probably only set the provider once in an initializer and that's it. But without an example app, you'd largely have to just know the do's and don't of the current implementation. (e.g. only set the provider in an intializer)

One thing with ruby (actually only the MRI) is that it's a bit hard to see concurrency issues. Because of the GIL there's only a handful of situations where you have context switches, like blocking IO. But if we ever want to support TruffleRuby, we'd better be prepared for thread safety.

IMHO the minimal part of the API that would need to be threadsafe would be the global API and Clients. E.g. in a multi-process multi-thread environment - think puma - I'd expect each worker to have it's own client. If someone wants threadsafe hooks they need to synchronize them on their own.

I'd be very interested in your take on this, though.

I might be highly biased though, because I'm really coming from the rails corner of ruby :D

maxveldink commented 3 months ago

One thing with ruby (actually only the MRI) is that it's a bit hard to see concurrency issues.

Yes, this is primarily why I don't have experience with it; I just haven't encountered that many Ruby projects that use concurrency. I do think it's worthwhile to start thinking about/working on this. I think tackling concurrency in the context of a multi-threaded app (i.e. puma or falcon) in MRI is a good starting place versus targeting support for an alternative Ruby implementation would be cool. Maybe we add an example puma or falcon app so we can start trying to observe concurrency issues is a good start? What're your thoughts @mschoenlaub ? I'm going to start a new issue and move this conversation over there, and go ahead and merge this in.

maxveldink commented 3 months ago

https://github.com/open-feature/ruby-sdk/issues/137