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

WS Http Request Version Default To 1.1 #891

Merged

Conversation

aditya-agarwal-groww
Copy link
Contributor

No description provided.

@scottf scottf requested a review from brimworks April 18, 2023 16:56
@scottf
Copy link
Contributor

scottf commented Apr 18, 2023

@brimworks Does this seem like a reasonable change?

@brimworks
Copy link
Collaborator

I don't think this client supports chunked encoding and thus it really doesn't support http 1.1. Maybe the request filtering mechanism could be modified to support changes to the http version so individual usage of this library could change the version?

@brimworks
Copy link
Collaborator

Here is a nice summary of http 1.1 clients: https://www2.cs.uh.edu/~gnawali/courses/cosc6377-f12/p1/http.html#http1.1clients Technical these requirements are not met, which is why I don't think we should change the http version (by default).

@aditya-agarwal-groww
Copy link
Contributor Author

@brimworks For Websocket connections we need to have Http 1.1 , should we then use http 1.1 for that particular use case ?

@brimworks
Copy link
Collaborator

@aditya-agarwal-groww is correct and I am wrong. This should be HTTP 1.1 as per the RFC that defines websocks it states:

The method of the request MUST be GET, and the HTTP version MUST
be at least 1.1.

Source: https://datatracker.ietf.org/doc/html/rfc6455

Hopefully the unimplemented parts of HTTP 1.1 don't come back and bite us later.

@scottf
Copy link
Contributor

scottf commented Apr 27, 2023

@brimworks Thanks for stepping in here. @aditya-agarwal-groww I'll merge it when the build completes and that will build the next 2.16.11-SNAPSHOT. I'll try to do a full release within the next 2 weeks.

@aditya-agarwal-groww
Copy link
Contributor Author

@scottf Sure, thanks a lot !!

@aditya-agarwal-groww
Copy link
Contributor Author

hey @scottf for some reason coverage/coveralls job is still pending

@scottf
Copy link
Contributor

scottf commented Apr 28, 2023

hey @scottf for some reason coverage/coveralls job is still pending

Coverage doesn't work for forks. I'll be merging this, I've just been out of office.

@scottf scottf merged commit cd9d64a into nats-io:main Apr 28, 2023
@scottf scottf changed the title Http Request Version Default To 1.1 WS Http Request Version Default To 1.1 May 4, 2023
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

3 participants