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

Ensure h2 Websocket requests include :scheme #60

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

zarqman
Copy link
Contributor

@zarqman zarqman commented Sep 13, 2023

This PR causes :scheme to be properly populated for http2 Websocket requests.

:scheme is presently showing up to the server as an empty string (which is odd, since I'd expect it to be nil instead--I did not chase this down).

According to RFC 8441, section 4, bullet 2, :scheme must be present for h2 WS requests (which contrasts to other uses of h2 CONNECT, where :scheme is expected to be omitted).

This also adds tests for the presence and validity of authority, path, and protocol.

Types of Changes

  • Bug fix.

Contribution

@@ -90,14 +90,14 @@ def close
end
end

def connect(authority, path, headers: nil, handler: Connection, extensions: ::Protocol::WebSocket::Extensions::Client.default, **options, &block)
def connect(authority, path, scheme: @delegate.scheme, headers: nil, handler: Connection, extensions: ::Protocol::WebSocket::Extensions::Client.default, **options, &block)
Copy link
Member

Choose a reason for hiding this comment

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

This looks okay to me, but realistically speaking, is there ever going to be a case where we want to override the scheme in this way? We can always introduce an argument later, but maybe we can use @delegate.scheme on line 100 for now? WDYT?

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 don't presently have a concrete use case for the extra arg. It does pass through from def self.connect(..., **options) on line 55, so it's a little more versatile than at first glance.

Because of that pass through, my intention was to parallel the usage of Async::HTTP::Client where high-level method calls already support overriding scheme: apart from the endpoint. If that parallelism seems worth it, then perhaps keep it? Otherwise, it's no problem to remove the arg. I'll let you make the call. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def connect(authority, path, scheme: @delegate.scheme, headers: nil, handler: Connection, extensions: ::Protocol::WebSocket::Extensions::Client.default, **options, &block)
def connect(authority, path, headers: nil, handler: Connection, extensions: ::Protocol::WebSocket::Extensions::Client.default, **options, &block)
scheme = @delegate.scheme

@ioquatix ioquatix merged commit c320706 into socketry:main Dec 9, 2023
16 of 18 checks passed
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.

2 participants