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

Set stream protocol to HTTP 1.1 #219

Merged
merged 3 commits into from
Sep 17, 2020

Conversation

cyu
Copy link
Contributor

@cyu cyu commented Sep 13, 2020

Fixes issue #191

I also added a change to allow the endpoint to be configurable – this would allow me to proxy streams as well as the REST API.

Comment on lines 3 to 4
def stream(endpoint: nil, &receiver)
endpoint ||= streaming_endpoint
Copy link
Owner

Choose a reason for hiding this comment

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

This can just be the default value instead.

Suggested change
def stream(endpoint: nil, &receiver)
endpoint ||= streaming_endpoint
def stream(endpoint: streaming_endpoint, &receiver)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah – at the time there was a reason I didn't do it that way, but I can't think of it right now...

Likely, it was late and my mind wasn't on straight. In any case, changes made.

Comment on lines 45 to 52
private

TIMEOUT = 30

def streaming_endpoint
Async::HTTP::Endpoint.parse(streaming_endpoint_url, alpn_protocols: Async::HTTP::Protocol::HTTP11.names)
end

private

TIMEOUT = 30

Copy link
Owner

Choose a reason for hiding this comment

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

This gets included on the Vehicle class, so I would rather it stays private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timdorr yeah - I though it would be okay since it clarified itself as a streaming_endpoint.

I refactored it a bit and made both the url and endpoint values static members on the Stream module. I can easily duplicate this code as I'm configuring the proxy, but I'd rather pull the streaming url from the gem in case it ever changes.

This allows streams to run through a proxy server:

proxy_client = Async::HTTP::Client.new(Async::HTTP::Endpoint.parse("http://localhost:8888"))
original_endpoint = vehicle.streaming_endpoint
headers = { "proxy-authorization" => Protocol::HTTP::Header::Authorization.basic('username','password')  }
proxy_client.proxied_endpoint(endpoint, headers)
@cyu cyu force-pushed the cy/streaming-force-http11 branch from d2bea18 to 6314394 Compare September 16, 2020 14:02
@ioquatix
Copy link

Great.

@timdorr timdorr merged commit d6bb6ff into timdorr:master Sep 17, 2020
@timdorr
Copy link
Owner

timdorr commented Sep 17, 2020

I'll get this pushed out in a sec.

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.

3 participants