line / armeria

Your go-to microservice framework for any situation, from the creator of Netty et al. You can build any type of microservice leveraging your favorite technologies, including gRPC, Thrift, Kotlin, Retrofit, Reactive Streams, Spring Boot and Dropwizard.
https://armeria.dev
Apache License 2.0
4.8k stars 912 forks source link

Fix a possible deadlock when `OAuth2Client` is used with `RetryingClient` #5715

Closed ikhoon closed 4 months ago

ikhoon commented 4 months ago

Motivation:

When OAuth2Client fails to acquire a granted access token, it directly returns a failed response. As the response is returned, RequestLog is not completed.

RetryingClient waits for some properties of RequestLog to be completed. Therefore, the incomplete RequestLog causes RetryingClient to get stuck.

This PR is a temporary workaround to fix the problem. The deadlock phenomenon will occur easily if a decorator returns a response directly. I created #5714 in which I will try to solve this problem ultimately.

Modifications:

Result:

Fixed a bug where RetryingClient gets to deadlock when OAuth2Client fails to obtain an access token.

jrhee17 commented 4 months ago

Just to make sure, am I understanding correctly that this issue is still a problem even if RetryingClient isn't in the decorator chain? (logs not completing could potentially pose a problem not restricted to retrying)

github-actions[bot] commented 4 months ago

๐Ÿ” Build Scanยฎ (commit: 631a878e1532ea8cd794404bcf3ac61646e0c42a)

Job name Status Build Scanยฎ
build-windows-latest-jdk-21 โœ… https://ge.armeria.dev/s/vpggi5w3fsxhc
build-self-hosted-unsafe-jdk-8 โœ… https://ge.armeria.dev/s/5ky773jpyr5xg
build-self-hosted-unsafe-jdk-21-snapshot-blockhound โœ… https://ge.armeria.dev/s/q3dp7tkaqyrn4
build-self-hosted-unsafe-jdk-17-min-java-17-coverage โœ… https://ge.armeria.dev/s/bf5nnb222zvzq
build-self-hosted-unsafe-jdk-17-min-java-11 โœ… https://ge.armeria.dev/s/zltqipovbsev6
build-self-hosted-unsafe-jdk-17-leak โœ… https://ge.armeria.dev/s/nv2n5igzcifpw
build-self-hosted-unsafe-jdk-11 โœ… https://ge.armeria.dev/s/hkn6nump2mvue
build-macos-12-jdk-21 โœ… https://ge.armeria.dev/s/tklm7r3hevbm6
ikhoon commented 4 months ago

am I understanding correctly that this issue is still a problem even if RetryingClient isn't in the decorator chain? (logs not completing could potentially pose a problem not restricted to retrying)

Right. RequestLog isn't completed regardless of RetryingClient. So RequestLog related operations such as LoggingClient and MetricCollectingClient do not work with the failed request before.

ikhoon commented 4 months ago

I try to complete RequestLog if a response is returned by a decorator. I am not confident with the workaround though. I still need to come up with new ideas for the problem. https://github.com/line/armeria/pull/5714/files#diff-f2eb8203256e0ecc52bed39c1f10697323d973d9f15c83096214794378fb197fR174-R178

jrhee17 commented 4 months ago

Thanks! I was just curious while writing the weekly report. Will also take a look at the approach in the other PR later ๐Ÿ˜ƒ

minwoox commented 4 months ago

@ikhoon ๐Ÿ‘ ๐Ÿ‘ ๐Ÿ‘