[
https://issues.apache.org/jira/browse/HDDS-17441?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Gargi Jaiswal reassigned HDDS-17441:
------------------------------------
Assignee: Gargi Jaiswal
> 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
> Affects Versions: 2.3.0
> Reporter: Siyao Meng
> Assignee: Gargi Jaiswal
> Priority: Major
> 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]
