-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adds configuration option to set max_fast_inline #2406
Adds configuration option to set max_fast_inline #2406
Conversation
History.md
Outdated
@@ -9,6 +9,11 @@ | |||
* Refactor | |||
* Consolidate option handling in Server, Server small refactors, doc chang (#2389) | |||
|
|||
## 5.0.3 / 2020-10-02 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add any changelog entries to the "master" entry, above this one
lib/puma/const.rb
Outdated
@@ -124,7 +124,7 @@ module Const | |||
# we amortize the cost of going back to the reactor for a well behaved | |||
# but very "greedy" client across 10 requests. This prevents a not | |||
# well behaved client from monopolizing the thread forever. | |||
MAX_FAST_INLINE = 10 | |||
MAX_FAST_INLINE = ENV.fetch('MAX_FAST_INLINE', 10).to_i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cleaner to make this an attribute of the Server object, which is read from configuration.
We don't have any env variables setting constants right now, probably shouldn't start.
History.md
Outdated
@@ -9,6 +9,11 @@ | |||
* Refactor | |||
* Consolidate option handling in Server, Server small refactors, doc chang (#2389) | |||
|
|||
## 5.0.3 / 2020-10-02 | |||
|
|||
* Features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add changelog entries to the "master" entry above this one
Cheers @fdel15! Thanks for your contribution! Let's move this into the configuration object, like the other attributes on Server which get set as "options". |
Thanks for the feedback Nate. I should have time this weekend to make the requested changes. |
I originally had it working this way because an ENV variable is a string, so the INFINITY option that the issue suggested would not work. Since we are no longer using an ENV variable, we can allow users to set |
@fdel15 can you update the title and PR description here to reflect what is actually happening now? Could be discussed elsewhere, but I'll raise it now because the |
Also: I don't think CI should be skipped for this PR |
Updated.
I removed the CI skip. I added it because I didn't add any tests in the PR. I feel like now that I am doing more than just adding an ENV variable I should be adding tests, but I am not sure how to test this feature. Any guidance on this would be greatly appreciated. |
Thanks for the PR. This jumps around, so I won't inline it. In server.rb attr_reader :max_fast_inline and: if requests >= max_fast_inline One would normally prefer that the API only exposes methods and attributes that are required externally. Re History.md, you may need to rebase for that. Also, the code I've got for it only adds the link info. At present it doesn't add items to the log, as that would require exact tagging for what 'group' an entry is listed in. |
Agree with @MSP-Greg's feedback that we want to keep the interface private. |
This needs to be added to |
Feedback addressed. Good call about not exposing an internal attribute.
I think this is a good call as well. I added a |
@fedl15 Do you see the message 'This branch cannot be rebased due to conflicts'? You may need to remove the commits in your PR branch that are duplicated, update your forks' master branch, then rebase on master in your PR branch. I think that made sense. I can help with more info if needed. You'll notice all the files listed as changed above. It makes it hard to see what's changed when the master branch's commits are mixed in... |
ff43f07
to
cf7dc6a
Compare
I thought I fixed the conflicts when I rebased. I must've done something silly though that caused all the files to be listed as changed. Sorry for not catching that earlier. I cherry picked all my commits for the branch and force pushed them up. This should be good for review now. |
Solid. Thanks for seeing this one through! |
Whoops. I kinda forgot I wanted the next release to be a patchlevel. I may revert and then quickly re-revert this on Monday when we release. |
This reverts commit e8b9998.
…)"" This reverts commit e48d6f3.
Description
Closes #2361
Adds a config option to set
max_fast_inline
and override theMAX_FAST_INLINE
default. You can set this variable toFloat::Infinity
to address use cases that wish to maintain the pre version 3.12.2 behavior (as discussed in the issue).Adds an ENV option to set MAX_FAST_INLINE constant. You can set this variable to zero to address use cases that wish to maintain the pre version 3.12.2 behavior (as discussed in the issue).I didn't add any tests because it is not clear to me where to add them. This change affects thePuma::Server#process_client
method, and it doesn't look like we have any tests set up at the moment. I also did not see any tests written when this change was introduced in the commits referenced in the issue v3.12.1...v3.12.2Your checklist for this pull request
[changelog skip]
or[ci skip]
to the pull request title.[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.