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
Changes from 1 commit
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
Next Next commit
Don't hang on 103
  • Loading branch information
yschimke committed Jul 15, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 01170c151958ebb93cc54139648330686748d8f4
Original file line number Diff line number Diff line change
@@ -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
}
}
}
7 changes: 6 additions & 1 deletion okhttp/src/main/kotlin/okhttp3/internal/http2/Http2Stream.kt
Original file line number Diff line number Diff line change
@@ -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
@@ -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