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

[4.x] Don't hang on a 103 response #7938

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -148,16 +148,18 @@ class CallServerInterceptor(private val forWebSocket: Boolean) : Interceptor {
}
}

private fun shouldIgnoreAndWaitForRealResponse(code: Int): Boolean = when {
// Server sent a 100-continue even though we did not request one. Try again to read the
// actual response status.
code == 100 -> true
companion object {
fun shouldIgnoreAndWaitForRealResponse(code: Int): Boolean = when {
// Server sent a 100-continue even though we did not request one. Try again to read the
// actual response status.
code == 100 -> true

// Handle Processing (102) & Early Hints (103) and any new codes without failing
// 100 and 101 are the exceptions with different meanings
// But Early Hints not currently exposed
code in (102 until 200) -> true
// Handle Processing (102) & Early Hints (103) and any new codes without failing
// 100 and 101 are the exceptions with different meanings
// But Early Hints not currently exposed
code in (102 until 200) -> true

else -> false
else -> false
}
}
}
13 changes: 9 additions & 4 deletions okhttp/src/main/kotlin/okhttp3/internal/http2/Http2Stream.kt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package okhttp3.internal.http2
import okhttp3.Headers
import okhttp3.internal.EMPTY_HEADERS
import okhttp3.internal.assertThreadDoesntHoldLock
import okhttp3.internal.http.CallServerInterceptor
import okhttp3.internal.http.CallServerInterceptor.Companion.shouldIgnoreAndWaitForRealResponse
import okhttp3.internal.notifyAll
import okhttp3.internal.toHeaderList
import okhttp3.internal.wait
Expand Down Expand Up @@ -258,12 +260,12 @@ class Http2Stream internal constructor(
if (this.errorCode != null) {
return false
}
if (source.finished && sink.finished) {
return false
}
this.errorCode = errorCode
this.errorException = errorException
notifyAll()
if (source.finished && sink.finished) {
return false
}
}
connection.removeStream(id)
return true
Expand All @@ -283,7 +285,10 @@ class Http2Stream internal constructor(
val open: Boolean
synchronized(this) {
if (!hasResponseHeaders || !inFinished) {
hasResponseHeaders = true
val status = headers[Header.RESPONSE_STATUS_UTF8]?.toIntOrNull()
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

if (status == null || !shouldIgnoreAndWaitForRealResponse(status)) {
hasResponseHeaders = true
}
headersQueue += headers
} else {
this.source.trailers = headers
Expand Down
11 changes: 7 additions & 4 deletions okhttp/src/test/java/okhttp3/InformationalResponseCodeTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,22 @@ class InformationalResponseCodeTest {
recordFrames = true
}

private var client = clientTestRule.newClient()
private var client = clientTestRule.newClientBuilder()
.followRedirects(false)
.build()

@Test
fun test103() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does master have this test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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()

Expand Down