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

Switch from Thin to Puma (port of smashing/puma_webserver branch) #93

Closed
wants to merge 2 commits into from

Conversation

johnnyshields
Copy link

@johnnyshields johnnyshields commented Dec 14, 2017

  • 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
@johnnyshields johnnyshields changed the title Squashed commit port of puma_webserver branch of Smashing: Switch from Thin to Puma (port of smashing/puma_webserver branch) Dec 14, 2017
@johnnyshields
Copy link
Author

Here is the diff I ported: Shopify/dashing@master...puma_webserver

@terraboops
Copy link

Oh no 😢 - we tried almost exactly this back in Dashing:
Shopify/dashing#677

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:
Shopify/dashing#677 (comment)

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
something custom we implement for SSE in Celluloid. This will allow us to keep total threads to a sane number, prevent context switching explosions and scale to many many many connected clients - far more than Thin would ever handle.

@terraboops
Copy link

terraboops commented Dec 15, 2017

For more details on what the celluloid implementation might look like:
sinatra/sinatra#1035 (comment)

@terraboops
Copy link

I'm gonna close this... Thanks again though!

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