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

6.0.0 #2918

Merged
merged 1 commit into from Oct 14, 2022
Merged

6.0.0 #2918

merged 1 commit into from Oct 14, 2022

Conversation

nateberkopec
Copy link
Member

No description provided.

Copy link
Member

@dentarg dentarg left a comment

Choose a reason for hiding this comment

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

Some comments, haven't done any comparison with any branch. Are we doing a release of everything in master now?

History.md Show resolved Hide resolved
History.md Outdated Show resolved Hide resolved
@MSP-Greg
Copy link
Member

Can we hold off a day or two to see about merging either #2892 or #2896?

They both fix use with Rack::BodyProxy.

Meanwhile, everyone can review HISTORY.md...

@MSP-Greg
Copy link
Member

MSP-Greg commented Aug 28, 2022

Drop Support for older Rubies? Maybe 2.2 & 2.3?

Ruby 2.6 is EOL, 2.7 is 'security maintenance'

Current release support:
2.6 Nokogiri
2.4 Nio4r
2.3 Rack 2
2.4 Rack 3 (unreleased)
2.5 Rails 6
2.7 Rails 7

See #2919

@nateberkopec
Copy link
Member Author

Are we doing a release of everything in master now?

Yes

@MSP-Greg
Copy link
Member

There are three bugs in the 6.0.0 list, I believe the last two are listed in 5.6.5?

I can update the link section in History...

@nateberkopec
Copy link
Member Author

Addressed Greg's comment above, waiting on #2896 and #2892

@nateberkopec
Copy link
Member Author

nateberkopec commented Sep 14, 2022

OK! Everything marked 6.0 is now closed... last call? Anyone? Anything else kicking around your head Greg?

EDIT: obv gonna update these first :)

@dentarg
Copy link
Member

dentarg commented Sep 14, 2022

Will 6.0 address #2921?

@MSP-Greg
Copy link
Member

Maybe add #2942? After that's merged, maybe add a PR that removes the following (tests may need changes):

puma/lib/puma/server.rb

Lines 50 to 53 in 5199ff2

# @deprecated v6.0.0
attr_writer :auto_trim_time, :early_hints, :first_data_timeout,
:leak_stack_on_error, :min_threads, :max_threads,
:persistent_timeout, :reaping_time

I thought about adding ALPN for SSL, but wrk doesn't use it, so it can't be used to see what kind of performance difference it makes. It should lessen the network traffic needed to initialize an SSL connection, but not sure whether it's significant in today's world. Regardless, it wouldn't be a breaking change, so it could be added in 6.1.

@MSP-Greg
Copy link
Member

MSP-Greg commented Sep 20, 2022

A quick look thru some code showed the below:

  1. Should MIN_THREADS & MAX_THREADS be removed?
  2. Should WEB_CONCURRENCY change to PUMA_WEB_CONCURRENCY (or even PUMA_WORKERS)? Not sure if it's used by other apps.

def puma_options_from_env
min = ENV['PUMA_MIN_THREADS'] || ENV['MIN_THREADS']
max = ENV['PUMA_MAX_THREADS'] || ENV['MAX_THREADS']
workers = ENV['WEB_CONCURRENCY']
{
min_threads: min && Integer(min),
max_threads: max && Integer(max),
workers: workers && Integer(workers),
environment: ENV['APP_ENV'] || ENV['RACK_ENV'] || ENV['RAILS_ENV'],
}
end

@MSP-Greg
Copy link
Member

Things that will break:

Rails: actioncable/test/client_test.rb

@MSP-Greg
Copy link
Member

MSP-Greg commented Sep 20, 2022

I totally forgot about changing to require_relative. Noticed it in some code, then found #2902.

EDIT: see PR #2964

@dentarg
Copy link
Member

dentarg commented Sep 20, 2022

2. Should WEB_CONCURRENCY change

I don't think it should change name, it could be breaking in the opposite way that Puma 5.0 was.

Puma 5.0 introduced the use of the WEB_CONCURRENCY ENV in the defaults in #2143 (maybe inspired by the Heroku documentation on Puma?) The possible ramifications on Heroku wasn't understood until after release though: #2393 (comment)

@dentarg
Copy link
Member

dentarg commented Sep 20, 2022

Things that will break:

Rails: actioncable/test/client_test.rb

Puma::Server#min_threads, Puma::Server#max_threads used here too: https://github.com/teamcapybara/capybara/blob/f7ab0b5cd5da86185816c2d5c30d58145fe654ed/lib/capybara/registrations/servers.rb#L43

History.md Show resolved Hide resolved
@MSP-Greg
Copy link
Member

Server's initialize method signature has changed between 6.0.0 and 5.6.5

6.0.0 - def initialize(app, log_writer=LogWriter.stdio, events=Events.new, options = {})

5.6.5 - def initialize(app, events=Events.stdio, options={})

Wondering if we should make the log_writer part of options, then revert the method signature change? Thoughts?

@dentarg
Copy link
Member

dentarg commented Sep 20, 2022

Sounds like a good idea to me

@MSP-Greg
Copy link
Member

(@MSP-Greg) Wondering if we should make the log_writer part of options, then revert the method signature change? Thoughts?

(@dentarg) Sounds like a good idea to me

@dentarg & @nateberkopec - see PR #2965.

@nateberkopec
Copy link
Member Author

Good catch. It's of course a major version release so we can break it if we want but it does feel unnecessary. So I'm in favor of changing the signature back.

@nateberkopec
Copy link
Member Author

@MSP-Greg re: your comments on env variables, we'll keep those the same. As @dentarg mentioned, WEB_CONCURRENCY especially is a wider convention among Heroku apps.

I'm OK with our other new config-related breakages we're introducing, but as @dentarg suggested we should spell them out individually.

@MSP-Greg
Copy link
Member

we should spell them out individually.

I was looking at that also, no time today to work on it. I believe both 6.0.0 and 5.6.5 set min & max threads from the opts parameter in Server#intialize, so Capybara could either delete the line or guard it with Server.respond_to? :min_threads=.

Rails will need to change its code, but QED. I'll check Capybara & Rails tomorrow with master.

@MSP-Greg
Copy link
Member

MSP-Greg commented Sep 22, 2022

Speaking of breaking changes, below is what I know of that's affected:

EDIT: Any thoughts on releasing an rc? Maybe with a caveat of 'normal CLI use'? Give more time to finish the docs and check the 'other use' repos like the above...

6.0-Upgrade.md Outdated Show resolved Hide resolved
@MSP-Greg
Copy link
Member

The flurry of PR's resulted from running a lot of CI to address the three repos (listed above) use of Puma in a nonstandard way.

@MSP-Greg
Copy link
Member

MSP-Greg commented Sep 26, 2022

Just piled PR's #2966, #2967, #2968, #2969, #2971 into one branch, ran the Test workflow seven times.

All MRI jobs passed, with one failure in 'NON-MRI: macos-11 jruby', which was a Java SSL error (I think), and one failure in (guarded) 'NON-MRI: macos-11 truffleruby' which was 'truffleruby: Unexpected internal exception in at_exit,'

Hence, I think the intermittent failure rate will drop considerably...

This weekend I did see a few test timeouts, but the duration of the compile step was several times longer than normal jobs. May see if that can be picked up by Actions...

Edit: I'm fine with committing the PR's, if anyone has time for a quick review, that would be appreciated.

@nateberkopec
Copy link
Member Author

todd-trapani-QldMpmrmWuc-unsplash

(image from unsplash, posting here for permanent URL to put in upgrade guide)

@nateberkopec
Copy link
Member Author

OK phew. I'm back, was busy moving into a new place the last two weeks.

@nateberkopec
Copy link
Member Author

OK, looks good to me. Would appreciate everyone's review.

@MSP-Greg
Copy link
Member

MSP-Greg commented Oct 10, 2022

Will review. Question - should we mark some classes as internal only? Maybe Client, Worker classes, etc?

I did try to determine what repos were using Puma in a non-standard way, and all the ones I could find were creating a server to send test requests to. IOW, bypassing Launcher, Runner, etc. So, most the Config related classes and Server.

I expect changes to Client (timers, IOBuffer, etc) and maybe others, and saving those changes until v7 would be a PITA...

Thoughts?

@nateberkopec
Copy link
Member Author

Will review. Question - should we mark some classes as internal only? Maybe Client, Worker classes, etc?

Sure, we can start doing that. What mechanism do you propose? Rails used :nodoc: IIRC.

@MSP-Greg
Copy link
Member

See #2988 for a PR. Looking for input as to what classes/modules should be considered 'private'.

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

4 participants