fix: use original response timeout in DEADLINE_EXCEEDED description - #6910
fix: use original response timeout in DEADLINE_EXCEEDED description#6910mmustafasenoglu wants to merge 3 commits into
Conversation
RetryingClient modifies ctx.responseTimeoutMillis() via SET_FROM_NOW with the remaining time, which loses sub-millisecond precision due to NANOSECONDS.toMillis() flooring. This causes the DEADLINE_EXCEEDED status description to report a smaller timeout than originally configured. Store the original response timeout as an attribute on the context before it gets modified, so ArmeriaClientCall can use it for the description. Fixes line#6894
When RetryingClient is used, ctx.responseTimeoutMillis() returns the remaining time (floored by NANOSECONDS.toMillis()), which causes the DEADLINE_EXCEEDED description to report a smaller timeout than configured. Read the original response timeout from the context attribute stored by AbstractRetryingClient and use it for the status description. Fixes line#6894
|
|
📝 WalkthroughWalkthroughThe retrying client now preserves the initially configured response timeout. gRPC deadline-exceeded descriptions use this value instead of a reduced timeout caused by retry processing. ChangesResponse timeout preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Tick the box to add this pull request to the merge queue (same as
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java (1)
559-568: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both timeout sources.
Add one JUnit 5 and AssertJ test through
RetryingClientthat asserts3000000000nsfor a3000ms timeout. Add one test without the attribute that verifies the fallback toctx.responseTimeoutMillis().The supplied
GrpcClientTest.deadlineInFuturetest only checks that the description contains"deadline exceeded after".As per path instructions, add coverage for the original timeout and fallback behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java` around lines 559 - 568, Add JUnit 5 and AssertJ regression tests through RetryingClient covering both timeout sources: assert a 3000 ms original timeout produces “3000000000ns”, and add a separate case without ORIGINAL_RESPONSE_TIMEOUT_MILLIS verifying the description uses ctx.responseTimeoutMillis(). Extend GrpcClientTest.deadlineInFuture or add focused tests while preserving its existing assertions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient.java`:
- Around line 95-97: Capture ctx.responseTimeoutMillis() once before
constructing State, then reuse that local value for both the State constructor
and ORIGINAL_RESPONSE_TIMEOUT_MILLIS attribute in AbstractRetryingClient.
Preserve the existing state and attribute initialization behavior while ensuring
both receive the identical original timeout.
In
`@grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java`:
- Line 663: Remove the redundant trailing blank line after the closing brace in
ArmeriaClientCall.java, while preserving the required single final newline at
end of file.
- Around line 559-568: The DEADLINE_EXCEEDED description in ArmeriaClientCall
must not report zero or Long.MAX_VALUE as the elapsed timeout. Update the
timeout selection using AbstractRetryingClient.ORIGINAL_RESPONSE_TIMEOUT_MILLIS
and ctx.responseTimeoutMillis() to use a finite per-attempt timeout, or an
established human-readable fallback when no finite timeout is available, before
converting it to nanoseconds.
---
Nitpick comments:
In
`@grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java`:
- Around line 559-568: Add JUnit 5 and AssertJ regression tests through
RetryingClient covering both timeout sources: assert a 3000 ms original timeout
produces “3000000000ns”, and add a separate case without
ORIGINAL_RESPONSE_TIMEOUT_MILLIS verifying the description uses
ctx.responseTimeoutMillis(). Extend GrpcClientTest.deadlineInFuture or add
focused tests while preserving its existing assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ec4f7d2-f02d-4c45-8cbe-d8015c8cfbbf
📒 Files selected for processing (2)
core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient.javagrpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java
| final State state = new State(config, ctx.responseTimeoutMillis()); | ||
| ctx.setAttr(STATE, state); | ||
| ctx.setAttr(ORIGINAL_RESPONSE_TIMEOUT_MILLIS, ctx.responseTimeoutMillis()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Capture the response timeout once.
ctx.responseTimeoutMillis() is read on Line [95] and again on Line [97]. If it reports remaining milliseconds, the second read can cross a millisecond boundary. The attribute can then be smaller than the timeout used to initialize State.
Proposed fix
- final State state = new State(config, ctx.responseTimeoutMillis());
+ final long responseTimeoutMillis = ctx.responseTimeoutMillis();
+ final State state = new State(config, responseTimeoutMillis);
ctx.setAttr(ORIGINAL_RESPONSE_TIMEOUT_MILLIS, ctx.responseTimeoutMillis());- ctx.setAttr(ORIGINAL_RESPONSE_TIMEOUT_MILLIS, ctx.responseTimeoutMillis());
+ ctx.setAttr(ORIGINAL_RESPONSE_TIMEOUT_MILLIS, responseTimeoutMillis);This protects the PR objective of reporting the original timeout value.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final State state = new State(config, ctx.responseTimeoutMillis()); | |
| ctx.setAttr(STATE, state); | |
| ctx.setAttr(ORIGINAL_RESPONSE_TIMEOUT_MILLIS, ctx.responseTimeoutMillis()); | |
| final long responseTimeoutMillis = ctx.responseTimeoutMillis(); | |
| final State state = new State(config, responseTimeoutMillis); | |
| ctx.setAttr(STATE, state); | |
| ctx.setAttr(ORIGINAL_RESPONSE_TIMEOUT_MILLIS, responseTimeoutMillis); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient.java`
around lines 95 - 97, Capture ctx.responseTimeoutMillis() once before
constructing State, then reuse that local value for both the State constructor
and ORIGINAL_RESPONSE_TIMEOUT_MILLIS attribute in AbstractRetryingClient.
Preserve the existing state and attribute initialization behavior while ensuring
both receive the identical original timeout.
| final long timeoutMillis; | ||
| final Long originalTimeout = | ||
| ctx.attr(AbstractRetryingClient.ORIGINAL_RESPONSE_TIMEOUT_MILLIS).get(); | ||
| if (originalTimeout != null && originalTimeout > 0) { | ||
| timeoutMillis = originalTimeout; | ||
| } else { | ||
| timeoutMillis = ctx.responseTimeoutMillis(); | ||
| } | ||
| status = status.augmentDescription("deadline exceeded after " + | ||
| MILLISECONDS.toNanos(ctx.responseTimeoutMillis()) + "ns."); | ||
| MILLISECONDS.toNanos(timeoutMillis) + "ns."); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
rg -n -C 5 'Long\.MAX_VALUE|responseTimeoutMillis\(\)|clearResponseTimeout|setResponseTimeoutMillis' \
--glob '*.java' || trueRepository: line/armeria
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -u
echo "Tracked files matching ArmeriaClientCall / AbstractRetryingClient:"
git ls-files | rg '(^|/)(ArmeriaClientCall|AbstractRetryingClient|ClientRequestContext).*\.java$|State.java' || true
echo
echo "Search relevant symbols:"
rg -n -C 4 'ORIGINAL_RESPONSE_TIMEOUT_MILLIS|responseTimeoutMillis|clearResponseTimeout|setResponseTimeoutMillis|Long\.MAX_VALUE|deadline exceeded after' . --glob '*.java' || trueRepository: line/armeria
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -u
echo "Candidate files:"
git ls-files | rg 'grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall\.java|core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient\.java|core/src/main/java/.*/ClientRequestContext\.java|core/src/main/java/.*/.*ResponseTimeout.*\.java' || true
echo
echo "ArmeriaClientCall relevant section:"
sed -n '530,590p' grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java 2>/dev/null | cat -n
echo
echo "AbstractRetryingClient relevant sections:"
sed -n '260,320p' core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient.java 2>/dev/null | cat -n
sed -n '1,80p' core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient.java 2>/dev/null | cat -n
echo
echo "ClientRequestContext declarations:"
rg -n -C 3 'default.*responseTimeoutMillis|clearResponseTimeout|setResponseTimeoutMillis|responseTimeoutMillis\(.*long|static.*MAX' core/src/main/java --glob '*.java' || true
echo
echo "Deadlock/deadline exceeded message usages:"
rg -n -C 3 'deadline exceeded after|ORIGINAL_RESPONSE_TIMEOUT_MILLIS|timeoutMillis = originalTimeout|clearResponseTimeout' --glob '*.java' || trueRepository: line/armeria
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -u
echo "Original timeout mutation around lines 160-210:"
sed -n '130,220p' core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient.java | cat -n
echo
echo "Whole retry timeout / attempt timeout methods around lines 70-130:"
sed -n '60,140p' core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient.java | cat -n
echo
echo "ClientRequestContext declarations and defaults around line 260:"
sed -n '220,285p' core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java | cat -n
echo
echo "Derived context timeout handling:"
rg -n -C 4 'newDerivedContext|RESPONSE_TIMEOUT_MILLIS|setResponseTimeoutMillis|clearResponseTimeout' core/src/main/java/com/linecorp/armeria/internal/client ClientUtil.java core/src/main/java --glob '*.java' | head -n 200
echo
echo "Behavioral calculation probe:"
python3 - <<'PY'
from datetime import timedelta
MAX = 9223372036854775807
print("MILLISECONDS.toNanos(Long.MAX_VALUE) =", MAX)
print("nanos to millis =", MAX // 1_000_000)
print("nanos to days =", int(timedelta(seconds=MAX // 1_000_000_000).total_seconds()) // (24*3600))
PYRepository: line/armeria
Length of output: 36114
Use a finite timeout when reporting DEADLINE_EXCEEDED.
AbstractRetryingClient.State disables the retry timeout when responseTimeoutMillis() is 0 or Long.MAX_VALUE, but ArmeriaClientCall stores and later reports that same value. Report the per-attempt timeout, or a human-readable fallback, instead of appending 9223372036854775807ns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java`
around lines 559 - 568, The DEADLINE_EXCEEDED description in ArmeriaClientCall
must not report zero or Long.MAX_VALUE as the elapsed timeout. Update the
timeout selection using AbstractRetryingClient.ORIGINAL_RESPONSE_TIMEOUT_MILLIS
and ctx.responseTimeoutMillis() to use a finite per-attempt timeout, or an
established human-readable fallback when no finite timeout is available, before
converting it to nanoseconds.
| return simpleMethodName; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the redundant trailing blank line.
Line [663] adds an empty line after the closing brace. Keep the required final newline, but remove the extra blank line.
As per path instructions, keep formatting free of redundant blank lines.
Proposed fix
}
-🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java`
at line 663, Remove the redundant trailing blank line after the closing brace in
ArmeriaClientCall.java, while preserving the required single final newline at
end of file.
Source: Path instructions
Capture ctx.responseTimeoutMillis() once to avoid race condition between State construction and ORIGINAL_RESPONSE_TIMEOUT_MILLIS attribute. Remove redundant trailing blank line in ArmeriaClientCall.java.
|
Addressed CodeRabbit review findings:
Regarding the Long.MAX_VALUE concern: when no response timeout is configured ( |
🔍 Build Scan® (commit: 8d5d6d2) |
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
It is possible that users want to modify the timeout via APIs such as ClientRequestContext#setResponseTimeout - I think the message should still reflect the modifications of such APIs.
I interpret the cause of the issue is related to how RetryingClient maintains/computes timeout.
The overall deadline is computed once:
And each derived timeout is set on the original context:
However, the root context timeout doesn't really need to be modified - it is just acting a s a vessel for setting the derived ctx timeouts.
What do you think of:
- Disabling the timeout for the original ctx once
final ClientRequestContextExtension ctxExt = ctx.as(ClientRequestContextExtension.class);
if (ctxExt != null) {
ctxExt.responseCancellationScheduler().cancelScheduled();
}
- and computing the timeout for each derived ctx once created?
// Set the timeout on the derived context, not the root context.
if (responseTimeoutMillis == 0) {
derivedCtx.clearResponseTimeout();
} else {
derivedCtx.setResponseTimeoutMillis(TimeoutMode.SET_FROM_NOW, responseTimeoutMillis);
}
By doing so, the original intended timeout will remain untouched
3 participants
Fixes #6894
Summary
When RetryingClient decorates a gRPC client and responseTimeoutMillis is configured, the DEADLINE_EXCEEDED status description reports a smaller timeout than the one originally configured (e.g., 2999000000ns instead of 3000000000ns).
Root Cause
Fix
Changes