-
Notifications
You must be signed in to change notification settings - Fork 323
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
Switch from Thin to Puma (port of smashing/puma_webserver branch) #93
Conversation
johnnyshields
commented
Dec 14, 2017
•
edited
Loading
edited
- Switch server to puma, provide puma config and adapt command line
- configure puma using config/puma.rb config file
- add default puma.rb config file for new projects
- adapt dashing cli to use the config file and correctly handle -d (daemonize) arg.
- adapt cli_test.rb to the new setup
- remove tmp dir from gitignore as we want to generate it in the project template
- add tmp/pids/ dir in project template (for saving puma pid / state files.
- Properly fix SSE connections in Puma!
- Realized that Rufus can also be multi-threaded now
- Added a mutex for each connection, allowing safe access during send_event
- Don't need to track a queue of events per connection - should lower memory footprint
- Connection termination detected by catching Puma::ConnectionError exceptions during send_event conntion writing
- Updated tests accordingly
- Modify SSE connection handling for Puma
- Keep track of events on per-client basis
- Clients get a UUID
- If a client's event queue grows too big it gets dropped
- If a Puma thread tries to send the next batch of events to a client and it's event queue is dropped, the queue will be created
- Updated tests to match above changes
Switch server to puma, provide puma config and adapt command line - configure puma using config/puma.rb config file - add default puma.rb config file for new projects - adapt dashing cli to use the config file and correctly handle -d (daemonize) arg. - adapt cli_test.rb to the new setup - remove tmp dir from gitignore as we want to generate it in the project template - add tmp/pids/ dir in project template (for saving puma pid / state files. - Properly fix SSE connections in Puma! - Realized that Rufus can also be multi-threaded now - Added a mutex for each connection, allowing safe access during send_event - Don't need to track a queue of events per connection - should lower memory footprint - Connection termination detected by catching Puma::ConnectionError exceptions during send_event conntion writing - Updated tests accordingly - Modify SSE connection handling for Puma - Keep track of events on per-client basis - Clients get a UUID - If a client's event queue grows too big it gets dropped - If a Puma thread tries to send the next batch of events to a client and it's event queue is dropped, the queue will be created - Updated tests to match above changes
Here is the diff I ported: Shopify/dashing@master...puma_webserver |
Oh no 😢 - we tried almost exactly this back in Dashing: First of all, thank you so much for your contribution. However, I don't know if we wan't to go down this road again... I think this comment sums up the issues with this approach nicely - basically we don't really get the benefit of Puma if we have to wrap everything in a mutex - but we do get a ton of weird complexity: To elaborate further, once you start hitting a decent number of connected clients the puma threads start doing so much context switching that they blow up and sometimes deadlock. By decent number of connected clients - I mean something like 2-3 connected clients per puma worker thread (so on average this is an upper bound of about 40 clients). To be totally honest, I think this is a reasonable upper bound as this isn't meant to be a super high scale "enterprise-grade" app. However - we can hit 40 connected clients easy-peasy already with Thin, so I don't see the benefit of going to Puma given the complexity of mutexing all-the-things. I think the correct solution is to either fix this in Sinatra (sinatra/sinatra#1035) or for us to break this one endpoint out of Sinatra entirely and have Puma hijack it and pass it off to |
For more details on what the celluloid implementation might look like: |
I'm gonna close this... Thanks again though! |