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

Introduce ForkTracker to automatically restart threads #344

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

artfuldodger
Copy link

@artfuldodger artfuldodger commented Jan 11, 2024

Summary

The Problem

When running puma or unicorn with multiple workers, they do some initial work in a process. When configured to preload the app, this can include the initialization of Optimizely::Project instances. When these objects are instantiated, there are threads that get started.

After the initial process is done with its work, it forks into new processes. This kills the threads.

This is documented here but requires updating the server config. Instead, making the gem responsible for re-starting the threads itself removes this footgun and makes the initial setup easier.

Side note: If there's no appetite for including this or a similar change in the gem itself, adding that documentation to the README and the Ruby quickstart guide would be helpful. It took us awhile to track down what was happening 😅

The Fix

This proof of concept copies over Rails' ActiveSupport::ForkTracker that hooks into Process._fork and allows us to re-start the threads automatically after forking. No extra configuration is needed by the user of the gem.

As an alternative approach, the DataDog gem handles this nicely as well and could be used as inspiration.

Test plan

In an app that includes the gem, start puma or unicorn with multiple workers and see that the Optimizely classes no longer need to be re-initialized after forking and the datafile continues to be updated, etc.

I'm happy to add some tests to this if you'd want to move forward with this implementation.

@andrewleap-optimizely
Copy link
Contributor

@artfuldodger thank you for bringing this to our attention and crafting this potential solution. We are going to add that warning from the docs to the readme for now and then discuss internally how to handle this PR. We'll keep you posted.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants