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

Dynamically resolve rack.multithread/rack.multiprocess #2288

Merged
merged 3 commits into from Jun 2, 2020
Merged

Dynamically resolve rack.multithread/rack.multiprocess #2288

merged 3 commits into from Jun 2, 2020

Conversation

FTLam11
Copy link
Contributor

@FTLam11 FTLam11 commented May 31, 2020

Closes #2174.

This PR dynamically resolves rack.multithread and rack.multiprocess.

  • 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] 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.

@nateberkopec
Copy link
Member

Thanks for your contribution. Ideally, the user should not have to add any config settings. We already know if their app multithread if the threadpool max size is > 1, and we know it's multiprocess if workers >= 1. So the extra config settings aren't necessary here IMO.

@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label May 31, 2020
@nateberkopec
Copy link
Member

Also, the values that need to change in response to config are here:

"rack.multithread".freeze => true,

The problem is that these values are frozen and constant.

@FTLam11 FTLam11 changed the title Add CLI options for multithread/multiprocess Dynamically resolve rack.multithread/rack.multiprocess Jun 1, 2020
@FTLam11
Copy link
Contributor Author

FTLam11 commented Jun 1, 2020

@nateberkopec Appreciate the explanation/hints, thank you!

Copy link
Member

@nateberkopec nateberkopec left a comment

Choose a reason for hiding this comment

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

Quick change then LGTM

lib/puma/binder.rb Outdated Show resolved Hide resolved
@FTLam11 FTLam11 marked this pull request as ready for review June 2, 2020 02:12
@nateberkopec nateberkopec merged commit dab5ff1 into puma:master Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix rack.multithread/rack.multiprocess
2 participants