Siyao Meng created HDDS-16973:
---------------------------------
Summary: 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
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]
