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

Remove nio from status server #2516

Merged
merged 3 commits into from Dec 17, 2020

Conversation

cjlarose
Copy link
Member

@cjlarose cjlarose commented Dec 17, 2020

Description

There's a bug related to #2018. Essentially, if the nio4r gem is ever loaded into the puma master process, it makes it impossible to do things like upgrade nio4r during a phased restart and also makes it impossible to remove the gems from disk without breaking phased restarts. Those problems were addressed mostly by my own #2427.

In PR #2427, I was pretty sure I made it so that if you're running a puma cluster with --prune-bundler, the puma master process would never load nio4r. It turns out that actually, if you enable the status server, the puma master process ends up loading nio4r -- the status server is launched in a thread in the master process. Since the status server just uses its own instance of Puma::Server, that ends up using a Reactor, which, in turn requires the nio4r gem.

The user impact is that if you're using the status server,

  1. you can't upgrade nio4r in a phased restart
  2. you can't safely delete the specific version of the nio4r native extensions from disk from previous releases
  3. worker processes unexpectedly double-load the nio4r gem. I don't know what the implications are for this last one, but in general, it's probably not good to double-load any gem.

There are a bunch of different creative solutions to this problem, but I think the easiest is probably to just disable queue_requests for the Puma::Server that serves the status server. Disabling that setting probably doesn't introduce a DOS vector since you probably shouldn't have your puma status server exposed to the public anyway--only trusted clients should have access.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

This is required in order to prevent the nio4r gem from being loaded
into the puma master process in a puma cluster.
@MSP-Greg MSP-Greg self-requested a review December 17, 2020 04:24
@cjlarose cjlarose merged commit 0475d70 into puma:master Dec 17, 2020
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.

None yet

2 participants