[jira] [Updated] (HDDS-16973) ECKeyOutputStream.close() does not end its flush thread after a writer failure or interrupt, leaking the thread or datanode client references

["Siyao Meng (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HDDS-16973?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Siyao Meng updated HDDS-16973:
------------------------------
    Attachment: MC-2-end-ec-flush-thread-in-close.patch

> 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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org