-
Notifications
You must be signed in to change notification settings - Fork 9.3k
Add support for HTTP/1.1 protocol upgrades #8849
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
Changes from all commits
889b147
9009b05
79e4373
6e337e3
2d4d678
ffce1d1
335d69e
dd09d8d
f2daee6
ab7c625
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,7 +225,7 @@ fun Response.promisesBody(): Boolean { | |
| } | ||
|
|
||
| val responseCode = code | ||
| if ((responseCode < HTTP_CONTINUE || responseCode >= 200) && | ||
| if ((responseCode < HTTP_CONTINUE || responseCode == HTTP_SWITCHING_PROTOCOLS || responseCode >= 200) && | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don’t think I agree that a protocol change is a body? This is used to drive stuff like the gzip interceptor, which seems inappropriate here?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The protocol change itself doesn't promise a body, but would depend on the actual protocl after a successful upgrade. |
||
| responseCode != HTTP_NO_CONTENT && | ||
| responseCode != HTTP_NOT_MODIFIED | ||
| ) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit unclear why this is enough to consider upgrades handled. Is this only for TCP? or do you think that all protocols would be handled as a Socket?
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Upgrade#upgrade_header_with_multiple_protocols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must admit that I only considered TCP upgrades and ignored other options. I suppose that the
socketwill only be necessary (and non-null) for upgrades which are not supported through another implementation (e.g. HTTP2). Returning the plain Socket might be considered as a generic way of supporting any protocol. That said: I'm unsure if this would make sense for OkHttp in general or if you prefer to keep supported protocols explicitly stated anywhere.So, would it be enough to improve the Javadoc to be more specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Jesse should weigh in. I don't think this is a simple feature to add especially given your TCP case is a poorly specced docker variant of a more general feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that on a protocol upgrade, OkHttp should hand off the
okio.Socketto the application layer, and it can do whatever new protocol it wants on top. Our responsibility for the upgrade ends and handing off the socket.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Websockets might be the only exception?