-
Notifications
You must be signed in to change notification settings - Fork 568
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
Don't retransmit a PipeDuplexRequestBody #2791
Conversation
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.
Nice! Love the inline explanations too. Thanks for looking into this!
// Give the mock server invalid actions (SendCompleted with no response message), which will | ||
// cause it to reset the socket connection. | ||
mockService.enqueue(ReceiveCall("/routeguide.RouteGuide/RecordRoute")) | ||
mockService.enqueue(SendCompleted) |
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.
A-ha, nice and cleaner than the initial attempts.
// Confirm we get an IOException from the broken connection and not an IllegalStateException | ||
// from incorrectly attempting to recover. | ||
val (requestChannel, deferredResponse) = routeGuideService.RecordRoute().executeBlocking() | ||
val e = assertFailsWith<IOException> { |
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.
Did you confirm that without your fix this was producing IllegalStateException
?
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.
Yes!
@@ -39,4 +39,7 @@ internal class PipeDuplexRequestBody( | |||
} | |||
|
|||
override fun isDuplex() = true | |||
|
|||
/** [Pipe.fold] can only be called once. */ | |||
override fun isOneShot() = true |
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.
good find
When is this change shipping to a new version? @oldergod |
No description provided.