Use actual thread local for Puma::Server.current.

[ioquatix]

The current implementation of Puma::Server.current will break inside fibers, e.g. lazy enumerators because it's fiber local, not thread local. If the intention is to have an actual thread local, this is the best fix.

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) 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.

[ioquatix]

It's probably worth noting that the current implementation conflicts with clean_thread_locals. In other words, it appears to me that enabling clean_thread_locals will break Puma::Server.current after the first request is handled. By storing puma_server in an actual thread local, this problem is avoided, since clean_thread_locals actually cleans fiber locals.

[ioquatix]

@nateberkopec can we merge this?

[dentarg]

Should we test this?

Can you show example code how/when this breaks? Just to educate us all further

[ioquatix]

Sorry, I should have added a test. Okay, included failing test + fix.

[ioquatix]

cc @nateberkopec @dentarg failing test included - fix included - ready for review :)

[MSP-Greg]

Isn't Thread.send(:attr_accessor, :puma_server) kind of global? Not sure if that might surprise some people, or affect code (outside of Puma) that doesn't expect it to be there?

[ioquatix]

Isn't Thread.send(:attr_accessor, :puma_server) kind of global? Not sure if that might surprise some people, or affect code (outside of Puma) that doesn't expect it to be there?

You are correct. I don't think it's particularly surprising, and I'm not sure there is a better approach that keeps things simple and efficient. No matter what, thread locals in Ruby have "global visibility". The fact that the name is prefixed puma_ ties it to the puma namespace, and I think that's safe enough, IMHO.

While I don't think it should influence our decision here, rails evaluated the different approaches and settled on the same design for the similar efficiency and simplicity reasons.

https://github.com/rails/rails/blob/20fe00d8abfce4aba34aab17db63964ddf658b83/activesupport/lib/active_support/isolated_execution_state.rb#L9

[MSP-Greg]

I'm not sure there is a better approach that keeps things simple

Agreed. Seeing the Rails example and who committed the code...

[ioquatix]

(@MSP-Greg as an aside, there was some discussion about introducing Thread::Local.new which is an unnamed object that stores thread local state, but it's extremely hard to implement correctly and probably needs to be done in the interpreter. See https://github.com/ruby-concurrency/concurrent-ruby/blob/master/lib/concurrent-ruby/concurrent/atomic/locals.rb and the related TheadLocalVar / FiberLocalVar for details of why it's complex).

[jpcamara]

I'm curious - what's the reason it can't be Thread.current.thread_variable_get and Thread.current.thread_variable_set instead? They operate exclusively at the thread level "and do not respect fibers" https://docs.ruby-lang.org/en/3.3/Thread.html#method-i-thread_variable_get

They also seem to support ruby going back pretty far, earlier into the 2 series.

[ioquatix]

Both approaches support all currently tested versions of Ruby.

As discussed above, Thread.current.puma_server is more efficient. There was a discussion here about it: rails/rails#43596 (comment)

[ioquatix]

Folks, what's the process for getting this reviewed / merged?

[MSP-Greg]

@ioquatix

I did approve this PR on 11-Apr...

Question, there's a method Puma.set_thread_name, and I believe almost all threads created by Puma are named. What if it was changed to the following, and we remove the code Thread.send(:attr_accessor, :puma_server) from puma/server.rb?

def self.set_thread_name(name)
  Thread.current.name = "puma #{name}"

  # This method was private on Ruby 2.4 but became public on Ruby 2.5+:
  Thread.current.singleton_class.send(:attr_accessor, :puma_server)
end

There seems to be concern about globally adding an attrribute to Thread, this would only do so with Puma threads.

Not sure if doing it this adds overhead (possibly creating a singleton class?). This passes the new test (locally on WSL2/Ubuntu 22.04).

[ioquatix]

Thanks @MSP-Greg

I appreciate your feedback.

I'm fine with your proposed change. Modifying singleton classes - however it means that if someone calls Puma::Server.current on a non-Puma thread, it will fail with NoMethodError. So we'd also need to add respond_to? or something similar, or be comfortable with NoMethodError on non-Puma threads (instead of nil).

Regarding approval / merge - I'm not able to merge it even with your approval.

Only those with write access to this repository can merge pull requests.

So I trust that we need someone else to actually decide where the bar for merging is. That is the part that's not clear to me. I think ideally someone is either (1) okay with this PR and merges it or (2) requests changes - so it's clear what needs to happen to move things forward.

[MSP-Greg]

however it means that if someone calls Puma::Server.current on a non-Puma thread, it will fail with NoMethodError.

True. But, it's set in the call to Puma::Server#process_client, so the non-Puma thread won't get the value.

Regardless, maybe setting it as a global attribute is best, since it won't throw an error if called from a non-Puma thread...

BTW, sorry about the mixup. Before I posted, you pinged @nateberkopec and @dentarg, so I wasn't sure if they'd look at it. Or, yes, I can merge it...

[ioquatix]

BTW, sorry about the mixup. Before I posted, you pinged @nateberkopec and @dentarg, so I wasn't sure if they'd look at it. Or, yes, I can merge it...

I don't know the normal process but it would be nice to have confidence that there was consensus on (1) fixing the bug and (2) the proposed fix.

In other words, I'd like to see this get merged, but I'd also like to follow whatever process is suitable for the project.

[dentarg]

As far as I know, there is no formal process :) Just people looking when they have time for it.

My hunch is that it is good to avoid NoMethodError, but I haven't spent much time thinking about this PR. I also pretty much trust you both @ioquatix @MSP-Greg so if you are in agreement, it is very much fine if Greg merges this.

[MSP-Greg]

@ioquatix @dentarg

Merged.

I was in the process of reviewing this, looking for the original reason for the method. The commit (July-2016) and issue were:
8370d2e
#932

I tried searching GitHub for Puma.current, and didn't seem to find anything. Using a thread local for it is something we may want to change, and with the expanded number of hooks available, it may be possible to remove it in Puma v7. Also, we may have servers for both 'listen' and control, I've haven't looked at issues like that, and we have servers in processes/workers...

Thanks, and sorry for the delay...

[ioquatix]

I'm 👍🏼 on dropping this interface in v7 especially if there are no obvious use cases. Maybe we can emit a warning in a v6.9... release.