[
https://issues.apache.org/jira/browse/HDDS-16745?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16745:
------------------------------
Attachment: CR-4-clear-unconsumed-call-id-and-recreate-refreshed-proxy.patch
> 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
> Priority: Critical
> 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]
