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.

Sign up for GitHub

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

Jump to bottom

Implement support for rack.protocol. #3399

Closed
wants to merge 1 commit into from
Closed

Implement support for rack.protocol. #3399

wants to merge 1 commit into from

Conversation

ioquatix
Copy link
Contributor

@ioquatix ioquatix commented Jun 5, 2024 •
edited
Loading

Description

Implement support for rack.protocol which is an optional feature of Rack 3.1, required to support protocol-websocket.

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.

@dentarg
Copy link
Member

dentarg commented Jun 5, 2024

Do you mind providing some more background on this? For anyone that hasn't followed along with the latest Rack developments. Any links to discussions/issues are appreciated too.

@ioquatix
Copy link
Contributor Author

ioquatix commented Jun 5, 2024

It has been discussed here: rack/rack#1946 and there are linked issues/discussions which provide more context.

ioquatix
ioquatix commented Jun 5, 2024
View reviewed changes
lib/puma/request.rb
headers['upgrade'] = protocol
headers['connection'] = 'upgrade'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could make these string constants if preferred.

@dentarg dentarg added feature waiting-for-review Waiting on review from anyone labels Jun 5, 2024
@ioquatix ioquatix mentioned this pull request Jun 5, 2024
Merged
@ioquatix ioquatix force-pushed the rack-protocol branch 2 times, most recently from 610c921 to 79c7ced Compare June 5, 2024 18:31
ioquatix
ioquatix commented Jun 5, 2024
View reviewed changes
lib/puma/request.rb
@@ -104,6 +108,12 @@ def handle_request(client, requests)
status, headers, app_body = [501, {}, ["#{env[REQUEST_METHOD]} method is not supported"]]
end

if protocol = headers.delete(RACK_PROTOCOL)
status = 101
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's advisable we force the status code here. HTTP/2 uses 200 Ok but HTTP/1 uses 101... There is no reason to use anything other than 101 AFAIK.

@ioquatix
Copy link
Contributor Author

ioquatix commented Jun 6, 2024 •
edited
Loading

Unrelated failure (last mentioned here):

TestPumaServer#test_chunked_request_invalid_extension_header_length = 0.00 s = .
(libev) kqueue kevent: Bad file descriptor
/Users/runner/work/_temp/139c090b-de4a-473c-b06a-14f67de45c34.sh: line 1:  6934 Abort trap: 6           test/runner --verbose
TestPumaServer#test_chunked_encoding =

That seems... odd?

@ioquatix ioquatix force-pushed the rack-protocol branch 3 times, most recently from 7011958 to d9c50ae Compare June 14, 2024 03:43
@ioquatix
Copy link
Contributor Author

Rack 3.1 was released with rack.protocol in the SPEC. It would be great to merge this PR.

@Earlopain Earlopain mentioned this pull request Jun 14, 2024
Open
3 tasks
@ioquatix
Copy link
Contributor Author

ioquatix commented Jun 15, 2024 •
edited
Loading

Actually, after consideration, I updated protocol-rack to support HTTP_UPGRADE as a fallback - if rack.protocol is not present - and it works okay for Puma. So this is completely optional. It's more of a problem if we support HTTP/2 - i.e. Falcon.

@ioquatix ioquatix closed this Jun 20, 2024
@ioquatix ioquatix deleted the rack-protocol branch June 20, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature waiting-for-review Waiting on review from anyone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ioquatix @dentarg