Siyao Meng created HDDS-16745:
---------------------------------
Summary: An OM client call that exits before issuing an RPC leaves
Client.callId set and fails every later call on that thread
Key: HDDS-16745
URL: https://issues.apache.org/jira/browse/HDDS-16745
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
CR-4-clear-unconsumed-call-id-and-recreate-refreshed-proxy.patch
h3. Mechanism
{{RetryInvocationHandler.Call#invokeMethod}}
({{RetryInvocationHandler.java:155-160}}) stores the call ID in the
{{Client.callId}} ThreadLocal before every attempt, with no {{finally}}. The
only thing that clears it is the {{Client.Call}} constructor
({{Client.java:244}}), and the next attempt asserts it is clear with
{{Preconditions.checkState(callId.get() == null)}} ({{Client.java:94}}). An
invocation that returns or throws without constructing a {{Client.Call}}, that
is without issuing an RPC, leaves the call ID behind.
{{FollowerReadInvocationHandler#invoke}} has such an exit.
{{OMProxyInfo#refreshAddressIfChanged}} sets the cached proxy to null by design
({{OMProxyInfo.java:219}}) so the next dial uses the new address, and the retry
policy calls it between two attempts of one request
({{OMFailoverProxyProviderBase.java:266}}, {{:536-551}}). The leader path
recreates the proxy through {{createOMProxyIfNeeded}}, but the follower loop
uses {{getCurrentProxy()}}, which returns the existing {{OMProxyInfo}} as is.
{{method.invoke(current.getProxy(), args)}} at
{{HadoopRpcOMFollowerReadFailoverProxyProvider.java:267}} then throws
{{NullPointerException}}, which is not an {{InvocationTargetException}} and
escapes the handler.
{{TestHadoopRpcOMFollowerReadFailoverProxyProvider.java:445-447}} already works
around the first half, disabling the call ID with the comment that it "is set
but not unset properly".
h3. Trigger
A client with {{ozone.client.follower.read.enabled}} and
{{ozone.client.failover.resolve-needed}} set, both of which default to false,
issues a read after the OM's IP address has changed, the case the second option
exists for. The first attempt fails with a connection error, the retry policy
re-resolves the address, and the next attempt dereferences the null proxy. One
thread, no concurrency, no node failure beyond the address change.
h3. Impact
The request that triggered the refresh fails instead of reaching the new
address, so the refresh does not help follower reads.
Worse, the calling thread is left unusable for OM RPC: every later call on it,
including one from an unrelated client instance to a healthy OM, fails with
{{IllegalStateException}} at {{Client.setCallIdAndRetryCount}}. Nothing clears
the ThreadLocal, so a pooled thread such as an S3 Gateway handler stays broken
until it is discarded.
h3. Reproduction
PASS. A real Hadoop RPC server stands in for the OM; the only injected state is
a stale cached address through {{OMProxyInfo#setCachedAddressForTest}}. After
the failed read, a second independent client on the same thread fails at
{{Client.java:94}}. Artifacts under {{confirmation/CR-4/}} and {{repro/}}.
The original candidate named three more exits from {{invoke}}. Those were
examined and found unreachable from production callers, so this issue covers
only the follower loop.
h3. Suggested fix
Make the call ID self cleaning: clear it in a {{finally}} in
{{RetryInvocationHandler.Call#invokeMethod}}, so that no present or future exit
can leave it behind. Separately, have the follower loop recreate the proxy a
refresh dropped, as the leader path already does.
h3. Patch
[^CR-4-clear-unconsumed-call-id-and-recreate-refreshed-proxy.patch], against
20e6a1c0f6039248373efd3eb1d7f2afcf7f1535.
{{Client}} gains {{clearCallId()}} and
{{RetryInvocationHandler.Call#invokeMethod}} calls it in a {{finally}}.
{{SET_CALL_ID_FOR_TEST}} existed only for the test workaround above, so both
are removed. {{HadoopRpcOMFollowerReadFailoverProxyProvider#getCurrentProxy}}
now goes through {{createOMProxyIfNeeded}}.
The existing {{TestHadoopRpcOMFollowerReadFailoverProxyProvider}} covers the
first half once its workaround is gone: 17 of its tests fail with
{{IllegalStateException}} without the {{finally}}. The new
{{testReadAfterDnsRefreshOfCurrentProxy}} in the same suite covers the second
half and fails with {{NullPointerException}} without the proxy change.
{{TestRetryProxy}} and the other OM failover proxy provider suites pass.
Checkstyle clean.
Found by TLA+ model checking and code review of the OM client retry cache and
at most once mutation paths under HDDS-15926, on commit
20e6a1c0f6039248373efd3eb1d7f2afcf7f1535. Checked against open HDDS issues for
duplicates before filing, in addition to the repository's merged git history.
Generated with Specula (Claude Opus 5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
