update URLMap Regexp usage for Ruby v3.3 #3165

fallwith · 2023-05-28T07:33:26Z

Ruby v3.3.0-preview1 doesn't support passing 3 arguments to Regexp.new, so update Puma::Rack::URLMap#remap to use a 2 argument version instead.

I didn't notice where Puma::Rack::URLMap#remap was currently tested and could use some advice on the testing front. Syntactically, I've tested the change going back as far as the current minimum supported Ruby, v2.4.

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.

Ruby v3.3.0-preview1 doesn't support passing 3 arguments to `Regexp.new`, so update `URLMap` to use a 2 argument version instead.

MSP-Greg · 2023-05-28T15:30:57Z

@fallwith

Thanks for the PR. Wondering if we should just pull in the code from Rack, as it was probably done in 5c9f871?

Not sure about using set, but that's an easy change. Would need to remove the constants require, and add:

  class URLMap
    HTTP_HOST   = 'HTTP_HOST'
    PATH_INFO   = 'PATH_INFO'
    SCRIPT_NAME = 'SCRIPT_NAME'
    SERVER_NAME = 'SERVER_NAME'
    SERVER_PORT = 'SERVER_PORT'

dentarg · 2023-05-28T15:39:57Z

Wondering if we should just pull in the code from Rack, as it was probably done in 5c9f871?

Sounds resonable

dentarg · 2023-05-28T17:00:09Z

I didn't notice where Puma::Rack::URLMap#remap was currently tested and could use some advice on the testing front.

Add a test app like this somewhere?

$ cat config.ru
map "/ok" do
  run ->(env) { [200, {}, ["OK"]] }
end

It currently blows up on Ruby 3.3 like this (when you don't have rack in the Gemfile) (as you already know, but including this for extra context)

arm64 $ b e puma
Puma starting in single mode...
* Puma version: 6.2.2 (ruby 3.3.0-p-1) ("Speaking of Now")
*  Min threads: 0
*  Max threads: 5
*  Environment: development
*          PID: 81381
! Unable to load application: ArgumentError: wrong number of arguments (given 3, expected 1..2)
bundler: failed to load command: puma (/Users/dentarg/.arm64_rubies/3.3.0-preview1/bin/puma)
/Users/dentarg/.arm64_rubies/3.3.0-preview1/lib/ruby/gems/3.3.0+0/gems/puma-6.2.2/lib/puma/rack/urlmap.rb:37:in `initialize': wrong number of arguments (given 3, expected 1..2) (ArgumentError)

        match = Regexp.new("^#{Regexp.quote(location).gsub('/', '/+')}(.*)", nil, 'n')
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	from /Users/dentarg/.arm64_rubies/3.3.0-preview1/lib/ruby/gems/3.3.0+0/gems/puma-6.2.2/lib/puma/rack/urlmap.rb:37:in `new'
	from /Users/dentarg/.arm64_rubies/3.3.0-preview1/lib/ruby/gems/3.3.0+0/gems/puma-6.2.2/lib/puma/rack/urlmap.rb:37:in `block in remap'
...

dentarg · 2023-05-28T17:05:44Z

Add a test app like this somewhere?

I suspect it isn't that easy, as the Gemfile in CI will always contain rack :/ and the faulty code in Puma will not be used

@fallwith how did you notice this issue? do you run any application using Puma where you haven't included rack in your bundle?

MSP-Greg · 2023-05-28T17:42:55Z

I think this also occurs with Ruby 3.2, as the change to Regexp happened there...

As to testing, we can probably get around the issue by disabling loading rack install in the Gemfile with an ENV setting, then using a test that loads Puma in a sub process.

I can investigate, but this isn't a good day for that, tomorrow is better...

dentarg · 2023-05-28T17:54:54Z

I think this also occurs with Ruby 3.2, as the change to Regexp happened there...

The example I tried (see above) does not blow up on Ruby 3.2.2-p53

dentarg · 2023-05-28T17:55:26Z

As to testing, we can probably get around the issue by disabling loading rack install in the Gemfile with an ENV setting, then using a test that loads Puma in a sub process.

Yes

MSP-Greg · 2023-05-29T03:28:08Z

The example I tried (see above) does not blow up on Ruby 3.2.2-p53

Thanks & sorry.

I didn't try it with 3.2, but saw the change in the doc's of the method signature.

I didn't notice that it was a deprecation, see rack/rack#1998

fallwith · 2023-05-30T04:47:22Z

Hi @MSP-Greg and @dentarg,

@fallwith how did you notice this issue? do you run any application using Puma where you haven't included rack in your bundle?

I work for New Relic and we use a suite of tests to ensure compatibility with Rack. When it comes to testing Puma, we dynamically generate 4 separate Gemfile files, one each for Puma versions 3, 4, 5, and 6. All of those Gemfile files contain both rack and rack-test alongside puma. We do, however, directly instantiate Puma::Rack::URLMap with one specific test, so we're always directly testing the code in question.

The New Relic Ruby agent began instrumenting Puma::Rack::URLMap 8 years ago in 2015 with this commit, and that's why we directly test the class.

If I can answer anything else or contribute any further content, please let me know.

dentarg · 2023-05-30T06:56:47Z

If I can answer anything else or contribute any further content, please let me know.

Please add a test as described above

With a `Gemfile` that excludes `rack`, have Puma use its own `URLMap` class to handle the mapping of the route defined in `config.ru`. Without the fix provided by PR 3165, this new test will time out.

fallwith · 2023-05-31T01:42:17Z

Test added with 8374a5b

MSP-Greg · 2023-05-31T14:42:21Z

Ruby head builds are broken due to Ruby changes that prevent nio4r from compiling. The Ubuntu-22.04 3.2 job failure is due to an intermittent, unrelated failure.

MSP-Greg · 2023-06-01T15:37:07Z

ruby head builds had an ABI change that nio4r didn't like. It's been reverted, and the head builds have been updated in GitHub Actions.

So, I reran the failing heads builds here.

nateberkopec

LGTM but since Greg and Dentarg worked on this more closely I'll leave it to you to merge.

MSP-Greg · 2023-06-06T02:28:56Z

@fallwith

Thanks for your work on this. The current test could run on Windows, but won't, due to the -w1 in the CLI args (workers require fork).

I updated that and a few other things. Patch is at 87294d45be. I can cherry-pick to your branch also...

fallwith · 2023-06-06T15:04:12Z

Thanks, @MSP-Greg. I have brought over your patch with d0a7e7e.

Just in case...

MSP-Greg · 2023-06-06T18:22:03Z

I added a check to make sure Rack is not loaded in the spawned Puma process. Unlikely to fail, but checking it verifies that the test is running in the proper environment. Prevents accidental failures in the future...

* test Ruby 3.3 with Puma (via the Rack suite) now that [puma/puma#3165](puma/puma#3165) has been merged * update `add_version` to respect _any_ prefix a human maintainer might want to specify for a version, and not just `=`

update URLMap Regexp usage for Ruby v3.3

617470e

Ruby v3.3.0-preview1 doesn't support passing 3 arguments to `Regexp.new`, so update `URLMap` to use a 2 argument version instead.

fallwith mentioned this pull request May 28, 2023

CI: Once Puma is known to work with Ruby v3.3, enable multiverse testing with that combination newrelic/newrelic-ruby-agent#2042

Closed

Add unit test for PR 3165

8374a5b

With a `Gemfile` that excludes `rack`, have Puma use its own `URLMap` class to handle the mapping of the route defined in `config.ru`. Without the fix provided by PR 3165, this new test will time out.

nateberkopec added the bug label Jun 1, 2023

nateberkopec approved these changes Jun 5, 2023

View reviewed changes

test_url_map.rb - run single

d0a7e7e

test/url_map_test/config.ru - check whether Rack::URLMap is defined

ffcc83e

Just in case...

MSP-Greg merged commit 188f5da into puma:master Jun 6, 2023
61 of 64 checks passed

fallwith mentioned this pull request Jun 6, 2023

enable Rack and Puma tests with Ruby 3.3 newrelic/newrelic-ruby-agent#2052

Merged

fallwith deleted the ruby33_compatibility branch June 7, 2023 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update URLMap Regexp usage for Ruby v3.3 #3165

update URLMap Regexp usage for Ruby v3.3 #3165

fallwith commented May 28, 2023 •

edited

MSP-Greg commented May 28, 2023

dentarg commented May 28, 2023

dentarg commented May 28, 2023 •

edited

dentarg commented May 28, 2023 •

edited

MSP-Greg commented May 28, 2023

dentarg commented May 28, 2023

dentarg commented May 28, 2023

MSP-Greg commented May 29, 2023

fallwith commented May 30, 2023

dentarg commented May 30, 2023

fallwith commented May 31, 2023

MSP-Greg commented May 31, 2023

MSP-Greg commented Jun 1, 2023

nateberkopec left a comment

MSP-Greg commented Jun 6, 2023 •

edited

fallwith commented Jun 6, 2023

MSP-Greg commented Jun 6, 2023

update URLMap Regexp usage for Ruby v3.3 #3165

update URLMap Regexp usage for Ruby v3.3 #3165

Conversation

fallwith commented May 28, 2023 • edited

Your checklist for this pull request

MSP-Greg commented May 28, 2023

dentarg commented May 28, 2023

dentarg commented May 28, 2023 • edited

dentarg commented May 28, 2023 • edited

MSP-Greg commented May 28, 2023

dentarg commented May 28, 2023

dentarg commented May 28, 2023

MSP-Greg commented May 29, 2023

fallwith commented May 30, 2023

dentarg commented May 30, 2023

fallwith commented May 31, 2023

MSP-Greg commented May 31, 2023

MSP-Greg commented Jun 1, 2023

nateberkopec left a comment

Choose a reason for hiding this comment

MSP-Greg commented Jun 6, 2023 • edited

fallwith commented Jun 6, 2023

MSP-Greg commented Jun 6, 2023

fallwith commented May 28, 2023 •

edited

dentarg commented May 28, 2023 •

edited

dentarg commented May 28, 2023 •

edited

MSP-Greg commented Jun 6, 2023 •

edited