Siyao Meng created HDDS-17441:
---------------------------------
Summary: Interrupted short-circuit GetBlock fails with
NullPointerException and leaves the received block file stream open in
XceiverClientShortCircuit
Key: HDDS-17441
URL: https://issues.apache.org/jira/browse/HDDS-17441
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
CR-6-release-short-circuit-stream-of-abandoned-request.patch,
TestBugCR6InterruptedReadLeaksBlockStream.java
h3. Mechanism
In {{XceiverClientShortCircuit.sendCommandWithTraceID}} an
{{InterruptedException}} from the wait for the response is caught and the
interrupt flag is restored, but neither a response nor an exception is
recorded, so {{Objects.requireNonNull(ioException)}} throws a
{{NullPointerException}}. {{BlockInputStream.getBlockDataUsingClient}} falls
back to gRPC only on {{IOException}}, so the NPE ends the read.
The request stays registered, the datanode still answers it, and
{{ReceiveResponseTask}} puts the block file stream it receives with the
response into {{blockStreamCache}} under the request's call ID. Only
{{BlockInputStream.getBlockDataUsingSCClient}} takes streams out of the cache
({{getFileInputStream}}), and only after {{getBlock}} returned normally, so
this stream is never claimed. {{close()}} does not touch the cache, and a
{{FileInputStream}} built from a received descriptor registers no cleaner, so
the descriptor stays open for the life of the process.
The same leak happens when a validator rejects a successful GetBlock response
after it arrived. The validators {{BlockInputStream}} uses do not reject such a
response for a normally written block (code reading).
h3. Trigger
A thread that reads a key through short-circuit read is interrupted while it
waits for the GetBlock response, for example a task cancelled with
{{Future.cancel(true)}} or an executor shut down with {{shutdownNow}}.
Short-circuit read must be enabled ({{ozone.client.read.short-circuit}}, off by
default).
h3. Impact
* The read fails with a {{NullPointerException}} instead of an {{IOException}},
so the gRPC fallback and the read retry handling are skipped.
* Each such read leaves one open descriptor of the block file in the client
process, also after {{OzoneClient.close()}}. Repeated cancellations accumulate
them toward the process descriptor limit, and they keep the space of deleted
block files in use (code reading for these two consequences).
h3. Reproduction
PASS on unmodified source. [^TestBugCR6InterruptedReadLeaksBlockStream.java]
({{hadoop-ozone/integration-test}}, needs the hadoop native library) starts a
one datanode MiniOzoneCluster with short-circuit read, writes one key, reads it
from a thread whose interrupt flag is set, closes the {{OzoneClient}} and looks
into the short-circuit client's stream cache by reflection. On unmodified
source the read fails with {{java.lang.NullPointerException}}, and after the
client is closed one stream with a valid descriptor is still in its cache. A
passing test means the defect is present. The reader sets its interrupt before
the read starts; an interrupt that arrives during the wait goes through the
same catch block (code reading). Run on macOS only.
h3. Patch
[^CR-6-release-short-circuit-stream-of-abandoned-request.patch], against
69d77beff60b9106e2517c36a7a1b2cc726af568. It also applies to master at
1acf315d546.
On an interrupt the client now records an {{InterruptedIOException}}, as
{{sendCommand}} already does for its own wait, so the call fails with an
{{IOException}} and the gRPC fallback runs (on the interrupted thread the
fallback also fails with an {{IOException}}). When the caller gives up on a
GetBlock (interrupted, or a validator rejects the response) it completes the
request's future exceptionally and then removes and closes any stream already
cached for the request. {{ReceiveResponseTask}} closes the stream itself when
completing the future fails because the caller already gave up. The receiver
caches the stream before it completes the future and the caller completes the
future before it looks in the cache, so one of the two always finds the stream,
and {{ConcurrentHashMap.remove}} hands it to only one of them, so it is closed
exactly once. Successful reads are unchanged.
{{TestXceiverServerDomainSocket.testClientReleasesStreamOfAbandonedRequest}}
covers both ways of giving up and a normal GetBlock on the same connection. It
records every stream the client receives (through a recording stream cache set
by reflection) and checks that the streams of the abandoned requests were
closed, not only removed from the cache. Without the change it fails because
the interrupted call throws a {{java.lang.NullPointerException}} instead of a
{{java.io.InterruptedIOException}}, and with that check relaxed it fails
because the abandoned request's stream is still in the cache. With the cache
removal kept but the two closes left out, it fails because the abandoned
streams are still open. With the change it passes, together with the rest of
{{TestXceiverServerDomainSocket}}, {{TestXceiverClientManagerSC}},
{{TestLocalChunkInputStream}}, {{TestDomainSocketFactory}},
{{TestBlockInputStream}} and {{TestClientThreadShutdown}}, and checkstyle on
{{hadoop-hdds/client}} and {{hadoop-ozone/integration-test}} is clean. With the
change the reproduction above fails at its first assertion: the interrupted
read fails with an {{IOException}} after the gRPC fallback, which sees the same
interrupt, was attempted.
The test also sets {{DefaultMetricsSystem.setMiniClusterMode(true)}} in the
suite setup, because the short-circuit client starts the metrics system, after
which a later {{ContainerMetrics.create}} in the same suite would fail with a
duplicate source name. The new test imports conflict textually with the patches
of HDDS-17436 and HDDS-17437 (adjacent AssertJ import lines); a rebase resolves
it.
Found by code review of the datanode short-circuit read paths, as part of the
TLA+ verification effort under HDDS-15926, on commit
69d77beff60b9106e2517c36a7a1b2cc726af568. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. Related but not
duplicates: HDDS-16517 (open pull request apache/ozone#11294, which simplifies
the client and also turns the interrupt into an {{InterruptedIOException}},
without releasing the stream), HDDS-16335 (open pull request apache/ozone#11225
on concurrent access in the same class, which changes
{{sendCommandWithTraceID}}, {{sendRequest}} and {{ReceiveResponseTask}}, so it
conflicts with this patch too), HDDS-16334 (changed the cache key type).
Merging with #11294 means: its interrupt handling replaces the first part of
this patch; it moves the validator loop from {{sendCommandWithTraceID}} into
{{sendCommand}}, so the release of the stream on a rejected response moves
there with it; and it makes {{sendCommandInternal}} private, so the new test
sends its delaying Echo another way. #11294 does not change
{{blockStreamCache}}. The attached patch is a proposal for review. Generated
with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
