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

Improves handling timeout/reset connection when reading request #2279

Merged
merged 4 commits into from Feb 11, 2022

Conversation

frojasg
Copy link
Collaborator

@frojasg frojasg commented Feb 10, 2022

  • Improves exception reporting when we failed to read a request stream
  • Creates RequestBodyException to wrap jetty exception when reading a
    request

* Improves exception reporting when we failed to read a request stream
* Creates RequestBodyException to wrap jetty exception when reading a
  request
@@ -241,6 +279,30 @@ class Http2ConnectivityTest {
}
}

class LockInterceptor constructor(val queue: ArrayBlockingQueue<Int>) : NetworkInterceptor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't remember exactly but do we still need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, this so the test can know when they can assert the side effect on the server side after the timeout.

"unknown",
"500"
).get().count.toInt()
).isEqualTo(1)
}

/** Confirm we don't page oncall and we record a 499 in the metrics. **/
@Test
fun clientTimeoutWritingTheRequest() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there might be value in keeping the old test, clientTimeoutWritingTheRequest, and adding a new one like clientCancelsWritingTheRequest(). The body of the new test should be what you have here, and the body of the timeout test should be what was here before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

@frojasg frojasg merged commit e110af2 into master Feb 11, 2022
@frojasg frojasg deleted the frojas/http2-connection-reset branch February 11, 2022 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants