Set Thread.current.puma_server in Thread init code, not every request#3774
Set Thread.current.puma_server in Thread init code, not every request#3774schneems merged 2 commits intopuma:masterfrom
Thread.current.puma_server in Thread init code, not every request#3774Conversation
joshuay03
left a comment
There was a problem hiding this comment.
I like the overall idea, just sharing my two cents on the implementation.
|
|
||
| @status = :run | ||
|
|
||
| @options[:puma_server] = self |
There was a problem hiding this comment.
Can we not pass this into the thread pool initialiser? It doesn't feel right to mutate the options at this stage.
There was a problem hiding this comment.
I think one test file (test_thread_pool.rb) creates a ThreadPool. I suppose we could change the method signature to the below or something similar:
def initialize(name, options = {}, server = nil, &block)
I'll look at changing it.
| # below is for CI | ||
| @server = options.respond_to?(:user_options) ? | ||
| options.user_options.delete(:puma_server) : | ||
| options.delete(:puma_server) |
There was a problem hiding this comment.
My personal opinion is that having to do this negates the micro optimisation gained from the core of this change (I don't think any lib code should exist just to satisfy CI). Could you link to the specific CI code that needs this? Is it just a naming conflict?
There was a problem hiding this comment.
negates the micro optimization gained from the core of this change
I'm not really concerned about code that runs once for each ThreadPool instance. I wanted to add it to options before the call, and delete it, but UserFileDefaultOptions isn't a hash, it just implements some methods of Hash.
Also, something that is currently set on every request is possibly more than a micro-optimization, and removing it (and doing it in the init) mean's one can't be sidetracked by the question "Why is this set in every request?". I really hate it when that happens...
There was a problem hiding this comment.
As an aside: I would like (someone) to remove all options = {} in favor of explicit KWARGS where possible into the future.
|
Just added a commit changing the ThreadPool init method signature. |
| require_relative 'io_buffer' | ||
|
|
||
| module Puma | ||
|
|
There was a problem hiding this comment.
Nit - feel free to ignore if this is your personal preference.
Description
Currently, the following is set in every call to
Server#process_request:PR moves code around to set it once in the
Thread.newblock.Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.