Do not send keep-alive in 101 (Switching Protocols) responses#709
Closed
rousskov wants to merge 1 commit intosquid-cache:masterfrom
Closed
Do not send keep-alive in 101 (Switching Protocols) responses#709rousskov wants to merge 1 commit intosquid-cache:masterfrom
rousskov wants to merge 1 commit intosquid-cache:masterfrom
Conversation
... because it breaks clients using websocket_client[1] library and is redundant in our HTTP/1.1 control messages anyway. I suspect that at least some buggy clients are confused by a multi-value Connection field rather than the redundant keep-alive signal itself, but let's try to follow RFC 7230 Upgrade example more closely this time and send no keep-alive at all. [1] https://pypi.org/project/websocket_client/
Contributor
Author
|
Folks suffering from this incompatibility problem have confirmed that the suggested change fixes it. |
Contributor
Author
|
@yadij, this PR undoes your (officially committed) suggestion so you may want to review it. |
squidadm
approved these changes
Aug 10, 2020
yadij
approved these changes
Aug 10, 2020
Contributor
Do we know how those folks have been testing the patch? live traffic or lab tests? FTR: Clients failing due to multi-value Connection fields will not be solved by this, only the amount of cases reduced. The cases my suggestion was for were not obeying HTTP/1.1 requirements about keep-alive based on the HTTP/1.1 label anyway, so breaking them is okay now we have a real-world traffic case needing to do so. |
yadij
changed the title
keep-alive in 101 (Switching Protocols) responses
yadij
added
the
M-cleared-for-merge
Contributor
Author
No, I do not have direct contact with those folks. My guess is that they are using live traffic at this point. |
squid-anubis
pushed a commit
that referenced
this pull request
Aug 12, 2020
... because it breaks clients using websocket_client[1] library and is redundant in our HTTP/1.1 control messages anyway. I suspect that at least some buggy clients are confused by a multi-value Connection field rather than the redundant keep-alive signal itself, but let's try to follow RFC 7230 Upgrade example more closely this time and send no keep-alive at all. [1] https://pypi.org/project/websocket_client/
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
yadij
pushed a commit
to yadij/squid
that referenced
this pull request
Aug 17, 2020
…cache#709) ... because it breaks clients using websocket_client[1] library and is redundant in our HTTP/1.1 control messages anyway. I suspect that at least some buggy clients are confused by a multi-value Connection field rather than the redundant keep-alive signal itself, but let's try to follow RFC 7230 Upgrade example more closely this time and send no keep-alive at all. [1] https://pypi.org/project/websocket_client/
squidadm
pushed a commit
to squidadm/squid
that referenced
this pull request
Aug 17, 2020
…cache#709) ... because it breaks clients using websocket_client[1] library and is redundant in our HTTP/1.1 control messages anyway. I suspect that at least some buggy clients are confused by a multi-value Connection field rather than the redundant keep-alive signal itself, but let's try to follow RFC 7230 Upgrade example more closely this time and send no keep-alive at all. [1] https://pypi.org/project/websocket_client/
yadij
pushed a commit
that referenced
this pull request
Aug 18, 2020
... because it breaks clients using websocket_client[1] library and is redundant in our HTTP/1.1 control messages anyway. I suspect that at least some buggy clients are confused by a multi-value Connection field rather than the redundant keep-alive signal itself, but let's try to follow RFC 7230 Upgrade example more closely this time and send no keep-alive at all. [1] https://pypi.org/project/websocket_client/
Contributor
Author
|
FTR: A relatively popular service, Google Voice at web.voice.telephony.goog, responds with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
... because it breaks clients using websocket_client[1] library and is
redundant in our HTTP/1.1 control messages anyway.
I suspect that at least some buggy clients are confused by a multi-value
Connection field rather than the redundant keep-alive signal itself, but
let's try to follow RFC 7230 Upgrade example more closely this time and
send no keep-alive at all.
[1] https://pypi.org/project/websocket_client/