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

Fix RealGrpcCall timeout #2816

Merged

Conversation

JGulbronson
Copy link
Contributor

LateInitTimeout doesn't actually work properly, and overwrites the new with the old, instead of the old with the new. I'm not convinced it's needed, and since it's internal to Wire, I'm hoping I can just delete it and use ForwardingTimeout instead.

Added a test to verify that the timeout is preserved after initCall is called.

@JGulbronson JGulbronson force-pushed the jgulbronson.fix-RealGrpcCall-timeout branch from 72c462b to 62e9398 Compare February 1, 2024 21:02
@@ -85,6 +86,9 @@ class GrpcClientTest {
private lateinit var incompatibleRouteGuideService: IncompatibleRouteGuideClient
private var callReference = AtomicReference<Call>()

// Just set to a large enough value we won't timeout in tests
private val okHttpClientTimeout = Duration.ofSeconds(117)
Copy link
Member

Choose a reason for hiding this comment

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

This value is SO BANANAS

Copy link
Member

Choose a reason for hiding this comment

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

🍌

Copy link
Member

@oldergod oldergod left a comment

Choose a reason for hiding this comment

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

Thank you

@@ -85,6 +86,9 @@ class GrpcClientTest {
private lateinit var incompatibleRouteGuideService: IncompatibleRouteGuideClient
private var callReference = AtomicReference<Call>()

// Just set to a large enough value we won't timeout in tests
private val okHttpClientTimeout = Duration.ofSeconds(117)
Copy link
Member

Choose a reason for hiding this comment

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

🍌

@oldergod oldergod merged commit 0180b78 into square:master Feb 2, 2024
7 checks passed
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