Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: synchronize provider registration #136

Merged

Conversation

mschoenlaub
Copy link
Contributor

This PR

  • synchronizes modifications to the global provider map

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.

@mschoenlaub mschoenlaub requested a review from a team as a code owner May 2, 2024 10:51
Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mschoenlaub mschoenlaub changed the title fix: synchronoze provider registration fix: synchronize provider registration May 2, 2024
Copy link
Member

@maxveldink maxveldink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't worked with threading a ton in Ruby, so maybe @technicalpickles or @josecolella have any thoughts here. This makes sense to me. 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).

@mschoenlaub
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

#137

@maxveldink maxveldink merged commit 1ff6fd0 into open-feature:main May 6, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants