armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Remove `RequestLog` dependency in `RetryingClient`

Open ikhoon opened this issue 1 year ago • 1 comments

Motivation:

Related: #5708, #4834

If a decorator directly returns a response, the RequestLog for the request won't be complete. Because the RequestLog is automatically completed when it starts to be written to the wire by AbstractRequestHandler.

In the case of RetryingClient, RequestLog's events are used to get exceptions to apply RetryRule. If the RequestLog events aren't complete, RetryingClient will fall into a deadlock.

Typically, RequestLog is automatically completed so it is difficult for the user to recognize that RequestLog must be completed. Consequently, it is easy to miss.

I propose two changes in this PR to solve the problem.

  1. If RequestLog.requestFirstBytesTransferredTimeNanos isn't logged when a response completes, we can assume that the response is returned by a decorator. A hook is added to a response completion event to complete the log manually for that case.

  2. RetryingClient is refactored not to retry on RequestLog but to use SplitHttpResponse and subscribe to headers. An exception when headers are received is used to apply RetryRule.

Modifications:

  • Add a hook that completes RequestLog if it is written to wire to ClientUtil
  • Split handleResponse into handleAggregatedResponse and handleStreamingResponse in RetryingClient
    • Remove RequestLog usage in handleStreamingResponse and use SplitHttpRequest to check if an exception is raised in the response.

Result:

  • Fixed deadlock that could occur in RetryingClient due to RequestLog not being completed.

ikhoon avatar Jun 01 '24 11:06 ikhoon

🔍 Build Scan® (commit: 8fa148322fdeb293ab366a894eedbfddad1d8c85)

Job name Status Build Scan®
build-ubicloud-standard-16-jdk-8 https://ge.armeria.dev/s/sd5ajsnkj57xo
build-ubicloud-standard-16-jdk-21-snapshot-blockhound ❌ (failure) https://ge.armeria.dev/s/hjoxju3hrnqfe
build-ubicloud-standard-16-jdk-17-min-java-17-coverage ❌ (failure) https://ge.armeria.dev/s/buok3kfrvfyxs
build-ubicloud-standard-16-jdk-17-min-java-11 https://ge.armeria.dev/s/76avtz3s5lsoi
build-ubicloud-standard-16-jdk-17-leak https://ge.armeria.dev/s/fxzvtiirv2huy
build-ubicloud-standard-16-jdk-11 https://ge.armeria.dev/s/euzvr2c22nb3m
build-macos-latest-jdk-21 https://ge.armeria.dev/s/a76grpdrbryw4

github-actions[bot] avatar Jun 01 '24 12:06 github-actions[bot]

This is ready for review.

ikhoon avatar Mar 31 '25 05:03 ikhoon