[
https://issues.apache.org/jira/browse/HDDS-16973?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16973:
------------------------------
Attachment: TestBugMC2EcFlushThreadParked.java
> ECKeyOutputStream.close() does not end its flush thread after a writer
> failure or interrupt, leaking the thread or datanode client references
> ---------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16973
> URL: https://issues.apache.org/jira/browse/HDDS-16973
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments: MC-2-end-ec-flush-thread-in-close.patch,
> TestBugCR3EcCloseInterruptLiveFlush.java, TestBugMC2EcFlushThreadParked.java
>
>
> h3. Mechanism
> {{ECKeyOutputStream}} writes stripes through a flush task on the client's
> write executor (threads {{client-write-TID-n}}). The task ends only when it
> takes the EOF mark from {{ecStripeQueue}} or sees {{closing}} after a
> {{take()}} returns. {{close()}} queues the EOF mark and waits for the task
> only inside {{if (!closing)}}, and its {{finally}} always releases the stream
> entries. No failing exit of {{close()}} cancels the task or waits for it:
> * {{closing}} was already set by a failed {{write()}}, or {{close()}} fails
> before the EOF mark is queued. Nothing is queued again and the task stays
> parked in {{take()}}.
> * The caller is interrupted in {{flushFuture.get()}}. The {{finally}}
> releases the stream entries while the task is still writing. {{closing}} is
> not set, so the task goes on with the queued stripes: on the cleared pool it
> asks OM for a new block, acquires datanode clients for it and writes the
> stripe there. Nothing releases those clients.
> A failure that starts on the flush thread (datanode or OM failure) is not
> affected, the task has ended by then.
> The parked thread is a regression from HDDS-10384, which moved the task from
> an executor per stream to the shared one and removed
> {{flushExecutor.shutdownNow()}} from the {{finally}} of {{close()}}. It is
> present in 1.4.1, 2.0.0 to 2.2.1 and master (read from the release tags, not
> run on them). 1.4.0 interrupted the flush thread but did not wait for it
> either, so the missing wait is older (what that left behind there was not
> checked).
> h3. Trigger
> An interrupt of the application thread while it is in {{write()}} or
> {{close()}} of an EC key, for example {{Future.cancel(true)}} on the task
> that writes. Nothing in Ozone's own client, file system, S3 Gateway, HttpFS,
> shell or freon code was found to interrupt such a thread, so it comes from
> the embedding application.
> The path needs no interrupt, any exception on the caller's thread in
> {{write()}} or {{close()}} takes it (reproduced with a mock encoder that
> throws), but no encoder shipped at this commit was found to throw for the
> stream's own buffers. HDDS-15341 (fixed in 2.2.0) was such an exception from
> the Java RS encoder, so 1.4.1 to 2.1.2 would have left the thread behind
> there too (from its stack trace and the tags, not run). On current code the
> trigger in practice is the interrupt.
> After a failed {{write()}} the thread stays only if the queue is empty at the
> task's next {{take()}}. The stray write needs a stripe that is still queued,
> which a slow datanode makes likely.
> h3. Impact
> * *Leaked thread.* One {{client-write-TID}} thread per affected stream stays
> parked until the client is closed (the executor is unbounded) and keeps the
> stream reachable, with one stripe of heap buffers (9 MiB for rs-6-3-1024k,
> from reading the code). Reproduced: 5 of 5 streams in each of three cases
> (failed write, interrupted write, interrupted close).
> * *Writes after a failed close, leaked datanode client references.*
> Reproduced with mock OM and datanode clients: after {{close()}} had thrown,
> the flush thread sent one {{AllocateBlock}}, acquired 5 more datanode clients
> and sent 3 {{WriteChunk}} and 5 {{PutBlock}} requests. 10 clients acquired, 5
> released. A real {{XceiverClientManager}} does not close a client with
> references left, so the connections would stay open (from reading the code,
> not run).
> No committed data is affected, the write has already failed for the caller.
> h3. Reproduction
> Attached, for {{hadoop-ozone/client}}, wired like {{TestOzoneECClient}} (real
> {{RpcClient}}, mock OM transport and datanode clients). Both pass on
> unmodified source while the bug is present. Path and command are in the class
> comments. Nothing was run against a real cluster.
> * [^TestBugMC2EcFlushThreadParked.java]: the parked thread.
> * [^TestBugCR3EcCloseInterruptLiveFlush.java]: the writes after an
> interrupted {{close()}}.
> h3. Patch
> [^MC-2-end-ec-flush-thread-in-close.patch], against
> 031e76aa2c9a60b67185390b5b33499cbbb82e43. It also applies to master at
> c5d4f28266dc7b2feb1954c235553afa88f91b5b.
> If the flush task is not done when the {{finally}} of {{close()}} is reached,
> {{close()}} now sets {{closing}} and cancels it
> ({{flushFuture.cancel(true)}}, the interrupt that {{shutdownNow()}} used to
> deliver). A cancelled future is done before its thread has stopped, so the
> task holds a lock while it runs and {{close()}} takes it before releasing the
> stream entries. The stripe retry loop stops when {{closing}} is set, and the
> {{InterruptedException}} catch restores the interrupt status.
> The normal path and a failure of the flush thread itself are unchanged. In
> the failing cases the caller gets the same exception, but {{close()}} returns
> only after the flush thread has stopped. That wait is on a monitor, so it has
> no timeout and a second interrupt does not end it. It is bounded because the
> flush thread reacts to its interrupt wherever it blocks ({{take()}} and the
> waits for datanode replies were run, the permit wait in
> {{XceiverClientGrpc.sendCommandAsync}} and the OM call were read).
> Three new cases in {{TestOzoneECClient}} fail without the change, and one of
> them fails without the lock alone. With the patch {{TestOzoneECClient}} (38
> tests), all of {{hadoop-ozone/client}} (162) and the mini cluster
> {{TestECKeyOutputStream}} (14) pass, checkstyle is clean.
> The patch for HDDS-16972 changes the same static import block of
> {{TestOzoneECClient}}, so whichever is applied second needs a trivial rebase.
> Found by TLA+ model checking and code review of the client EC key write and
> file checksum paths under HDDS-15926, on commit
> 031e76aa2c9a60b67185390b5b33499cbbb82e43. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-16271 (open, timeouts for the blocking future calls of the
> EC write path) and HDDS-15916 (resolved, encoder not released in
> {{close()}}). The attached patch is a proposal for review. Generated with
> Specula (Claude Opus 5.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]
