-
Notifications
You must be signed in to change notification settings - Fork 925
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
Complete DefaultClientRequestContext.whenIntialized()
after fully initializing a context
#3636
Conversation
…izing a context Motivation: `LazyDynamicEndpointGroupTest.emptyEndpoint()` sometimes failed in CI builds. line#3381 It expects to fail the test with `EmptyEndpointException`, however it fails with `ClosedStreamException`. After digging the issue, I find out that there is a race in `whenInitialized`. If the `acquiredEventLoop.execute()` is executed immediately and eailer than returning the method, https://github.com/line/armeria/blob/d4880fe12690d2dafd2c5e7fa9f24c3b24837a00/core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java#L307-L309 the callbacks of `ctx.whenInitialized()` will be invoked before a `RequestLog` is completed. https://github.com/line/armeria/blob/207c5e038f59802dca769936a50e219a5fe308ea/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java#L337-L348 As the `req` is closed already, the `req.write()` would be failed with `ClosedStreamException`. Modifications: - Complete `DefaultClientRequestContext.whenIntialized()` after `initContextAndExecuteWithFallback` of `ClientUtil` Result: - You no longer see `ClosedStreamException` when an `EndpointGroup` is empty. - Fixes line#3381
Codecov Report
@@ Coverage Diff @@
## master #3636 +/- ##
============================================
+ Coverage 73.83% 73.94% +0.10%
- Complexity 14378 14530 +152
============================================
Files 1263 1266 +3
Lines 54874 55349 +475
Branches 7021 7103 +82
============================================
+ Hits 40518 40929 +411
- Misses 10786 10801 +15
- Partials 3570 3619 +49
Continue to review full report at Codecov.
|
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.
What a brilliant!
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 find!
core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java
Outdated
Show resolved
Hide resolved
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.
Thanks, @ikhoon !
…nitializing a context (line#3636) Motivation: `LazyDynamicEndpointGroupTest.emptyEndpoint()` sometimes failed in CI builds. line#3381 It expects to fail the test with `EmptyEndpointException`, however, it fails with `ClosedStreamException`. After digging the issue, I find out that there is a race in `whenInitialized`. If the `acquiredEventLoop.execute()` is executed immediately and completed earlier than returning the method, https://github.com/line/armeria/blob/d4880fe12690d2dafd2c5e7fa9f24c3b24837a00/core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java#L307-L309 the callbacks of `ctx.whenInitialized()` will be invoked before a `RequestLog` is completed. https://github.com/line/armeria/blob/207c5e038f59802dca769936a50e219a5fe308ea/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java#L337-L348 As the `req` is closed already, the `req.write()` would be failed with `ClosedStreamException`. Modifications: - Complete `DefaultClientRequestContext.whenIntialized()` after `initContextAndExecuteWithFallback` of `ClientUtil` Result: - You no longer see `ClosedStreamException` when an `EndpointGroup` is empty. - Fixes line#3381
Motivation:
LazyDynamicEndpointGroupTest.emptyEndpoint()
sometimes failed in CIbuilds. #3381 It expects to fail the test with
EmptyEndpointException
,however, it fails with
ClosedStreamException
.After digging the issue, I find out that there is a race in
whenInitialized
.If the
acquiredEventLoop.execute()
is executed immediately and completed earlier thanreturning the method,
armeria/core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java
Lines 307 to 309 in d4880fe
the callbacks of
ctx.whenInitialized()
will be invoked before aRequestLog
is completed.
armeria/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java
Lines 337 to 348 in 207c5e0
As the
req
is closed already, thereq.write()
would be failed withClosedStreamException
.Modifications:
DefaultClientRequestContext.whenIntialized()
afterinitContextAndExecuteWithFallback
ofClientUtil
Result:
ClosedStreamException
when anEndpointGroup
is empty.