-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
[4.x] Don't hang on a 103 response #7938
Changes from all commits
01170c1
aa6f1d7
42027cd
713b9f3
15ebeee
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 |
---|---|---|
|
@@ -30,19 +30,22 @@ class InformationalResponseCodeTest { | |
recordFrames = true | ||
} | ||
|
||
private var client = clientTestRule.newClient() | ||
private var client = clientTestRule.newClientBuilder() | ||
.followRedirects(false) | ||
.build() | ||
|
||
@Test | ||
fun test103() { | ||
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. Does master have this test? 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. No, this is a manual test, see line 24. The problem is mockwebserver in 4.x doesn't have support for informational responses. So tests on master are better. |
||
// Pretend we are curl so cloudflare will send a 103 | ||
val request = Request.Builder() | ||
.url("https://tradingstrategy.ai") | ||
.header("user-agent", "curl/7.85.0") | ||
.url("https://theornateoracle.com/cart.php?action=add&product_id=456") | ||
.header("user-agent", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) " + | ||
"AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36") | ||
.build() | ||
|
||
val response = client.newCall(request).execute() | ||
|
||
assertThat(response.code).isEqualTo(200) | ||
assertThat(response.code).isEqualTo(302) | ||
assertThat(response.protocol).isEqualTo(Protocol.HTTP_2) | ||
response.close() | ||
|
||
|
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.
Do you mind retargeting
master
, then cherry-picking this back to okhttp_4x ?I’m a little surprised to see status codes handled at this layer
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 it went here so I could make the minimal change.
I can try again to apply some of the changes directly, it's been a while, so it's a bit vague why it broken down.
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 still not a fan of handling status codes at the protocol layer. Revert this part and keep the fix for the infinite wait?
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.
This feels unfortunate. I really would like to handle this properly in 4.12.