HDFS-17972. TestDFSClientRetries: restore the fault injector and release its latch when the deadlock test dies - #8712
Open
joseluisll wants to merge 1 commit into
Conversation
…ase its latch when the deadlock test dies
testLeaseRenewAndDFSOutputStreamDeadLock installs an anonymous
DFSClientFaultInjector into the static DFSClientFaultInjector.instance and
never restores it. That injector's delayWhenRenewLeaseTimeout() waits on a
CountDownLatch that is only counted down from SleepFixedTimeAnswer.answer(),
i.e. only if the mocked NameNode.complete() actually runs. The test's finally
block only shut the cluster down.
LeaseRenewer.run() calls delayWhenRenewLeaseTimeout() from inside
synchronized (this), on the SocketTimeoutException abort path, so a renewer
waiting in that injector holds the LeaseRenewer monitor for as long as it
waits. The test works because complete() releases the latch while
out1.close() is still running, before close() needs that monitor again.
If the write pipeline stalls, out1.close() never reaches completeFile(), so
complete() is never invoked and the latch stays at 1. The renewer then holds
the LeaseRenewer monitor indefinitely, and out1.close() deadlocks against it
on its own way out:
closeImpl() -> closeThreads() -> setClosed() -> DFSClient.endFileLease()
-> LeaseRenewer.addClient() (synchronized)
@timeout does not rescue this. In SAME_THREAD mode JUnit interrupts the test
thread at the deadline and then waits for the method to return, and a thread
blocked on a monitor is not interruptible, so the method never returns. The
surefire fork hangs until forkedProcessTimeoutInSeconds kills it, which loses
the results for the rest of the class.
Fix the test isolation:
- Bound the wait to 30 seconds. This is the part that actually breaks the
deadlock: the finally block below runs only after out1.close() returns, so
it cannot help a thread that is stuck inside close(). With the bound, the
renewer releases the monitor, close() completes, and a stalled run fails as
one ordinary test failure instead of hanging the fork. Also re-assert the
interrupt flag rather than printing the trace.
- Count the latch down in the finally, before restoring the injector and
before cluster.shutdown(), so the renewer is released immediately on the
normal path instead of waiting out the bound.
- Save the previous injector and restore it in that same finally, so the
static does not stay pointed at this test's subclass, matching the
convention used by TestPread, TestClientProtocolForPipelineRecovery,
TestDFSInputStream and TestPipelineCloseRecoveryByteArrayLeak. Restoring
alone would not be enough: it does nothing for a renewer already waiting
inside the old injector.
Verified by simulating the stall so that complete() is never reached: without
the bound the test hangs in out1.close() on the renewer monitor and never
returns; with it the test completes, the injector is restored and the latch
released. testLeaseRenewAndDFSOutputStreamDeadLock and
testLeaseRenewSocketTimeout both still pass in that order, in 10.3s and 4.1s,
so the normal path is unaffected.
This is a test-only change; the deadlock the test covers is unaffected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
https://issues.apache.org/jira/browse/HDFS-17972
TestDFSClientRetries#testLeaseRenewAndDFSOutputStreamDeadLockinstalls an anonymousDFSClientFaultInjectorinto the staticDFSClientFaultInjector.instanceand never restores it. The injector'sdelayWhenRenewLeaseTimeout()waits on aCountDownLatchwith an unboundedawait(), and that latch is counted down in exactly one place -SleepFixedTimeAnswer.answer()- which runs only if the mockedNameNode.complete()is invoked. The test'sfinallyblock only callscluster.shutdown().LeaseRenewer.run()calls the injector from insidesynchronized (this)on theSocketTimeoutExceptionabort path, so a renewer waiting there holds theLeaseRenewermonitor for the whole wait. The test normally passes becausecomplete()releases the latch whileout1.close()is still running.If the write pipeline stalls,
closeImpl()never reachescompleteFile(),complete()is never invoked, the latch stays at 1, and the renewer holds the monitor indefinitely.out1.close()then deadlocks against it on its own exit path:@Timeout(120)does not recover this. In JUnit's defaultSAME_THREADmode the deadline interrupts the test thread and then waits for the method to return, and a thread blocked on a monitor is not interruptible. The surefire fork therefore hangs untilforkedProcessTimeoutInSecondskills it, and the results for the remaining tests in the class are lost rather than reported. The static injector is also left pointing at the test's own subclass.Changes, all test-only:
finallyruns only afterout1.close()returns, which is exactly where the thread is stuck, so cleanup alone cannot help. With the bound, the renewer releases the monitor and a stalled run reports one ordinary test failure instead of hanging the fork.finally, ahead of restoring the injector and ahead ofcluster.shutdown(), so the renewer is released promptly on the normal path.finally, matching the convention inTestPread,TestClientProtocolForPipelineRecovery,TestDFSInputStreamandTestPipelineCloseRecoveryByteArrayLeak. Restoring alone is insufficient: it does nothing for a renewer already inside the old injector.e.printStackTrace().No production code is touched, and the deadlock this test guards (HDFS-9294) is unaffected.
How was this patch tested?
mvn -pl hadoop-hdfs-project/hadoop-hdfs test -Dtest=TestDFSClientRetries- 13 tests, 0 failures, 0 errors (JDK 21, Maven 3.9.16).testLeaseRenewAndDFSOutputStreamDeadLockandtestLeaseRenewSocketTimeoutalso pass together in that order in a single JVM.The failure was reproduced by driving the unmodified test method while withholding the DataNode's ack (
DataNodeFaultInjector.delaySendingAckToUpstream), soDataStreamer.waitForAckedSeqnoblocks andcomplete()is unreachable. Without this change the test method never returns after the JUnit interrupt andDFSClientFaultInjector.get()is still the test's own subclass; with it the method returns and the injector is restored.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
Contains content generated by Claude Code.
🤖 Generated with Claude Code