Siyao Meng created HDDS-17403:
---------------------------------
Summary: LeaseManager.shutdown can leave a live lease when acquire
runs concurrently, and can run a lease callback after it returns
Key: HDDS-17403
URL: https://issues.apache.org/jira/browse/HDDS-17403
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
MC-3-lease-manager-shutdown-excludes-acquire-and-waits-for-callbacks.patch,
TestBugMC3MC4LeaseManagerShutdown.java
h3. Mechanism
The Javadoc of {{LeaseManager.shutdown}} says all active leases are released
and their callbacks are not executed. Two races break that.
*Acquire during shutdown.* {{shutdown}} is not synchronized. It disables and
interrupts the monitor, loops over {{activeLeases.keySet()}} calling
{{release}}, and sets {{isRunning = false}} only after the loop. The
{{acquire}} overloads are synchronized but only check {{isRunning}}, so an
{{acquire}} that runs between two iterations of the loop, or that has passed
its status check when the loop starts, succeeds. The key iterator of the
{{ConcurrentHashMap}} does not see a key added to a bin it has already passed,
so that lease is never released. When {{shutdown}} returns, the lease is still
in the map, {{hasExpired()}} is false, {{renew}} succeeds, and it never expires
because the monitor has stopped.
*Callback after shutdown.* {{shutdown}} never waits for the monitor thread and
never shuts down the monitor's callback executor. If the monitor has already
found a lease past its deadline and is inside {{release}}, {{shutdown}} waits
for the lock, finds the lease gone, and returns. The monitor then hands the
callback to its executor, and the callback runs after {{shutdown}} has returned.
h3. Trigger
Any {{shutdown}} that overlaps an {{acquire}} or an expiry. In SCM the only
production user is {{CloseContainerEventHandler}}.
{{StorageContainerManager.stop}} closes the event queue first, but
{{EventQueue.close}} only calls {{ExecutorService.shutdown}} on its executors,
so queued {{CLOSE_CONTAINER}} events keep calling {{acquire}} while {{stop}}
goes on to {{leaseManager.shutdown}} (argued from the code, not run in an SCM).
A close container lease that expires around the same time takes the second path.
h3. Impact
No wrong outcome is visible in Ozone today. {{CloseContainerEventHandler}}
discards the returned {{Lease}}, {{get}} and {{release}} throw
{{LeaseManagerNotRunningException}} after shutdown, SCM never restarts the same
{{LeaseManager}}, and the late callback fires {{DATANODE_COMMAND}} into the
already closed event queue, which drops it. The defect is in a shared utility
whose documented contract does not hold: a caller that keeps the returned
lease, restarts the manager, or tears down state its callbacks use after
{{shutdown}} would be affected.
h3. Reproduction
[^TestBugMC3MC4LeaseManagerShutdown.java] is self contained (target path and
Maven command in the class comment) and uses only the public {{LeaseManager}}
API. The only timing aid is the resource's own {{hashCode()}}, which
{{LeaseManager}} calls inside {{acquire}} and {{release}}; it pauses that call
at a known point. On unmodified source both tests pass, 5 of 5 runs:
* {{acquireDuringShutdownLeavesLiveLease}}: an {{acquire}} paused after its
status check until {{shutdown}} waits for the lock. After {{shutdown}} returned
and another 500 ms: {{hasExpired=false remainingMs=-409 callbackRan=false}} for
a 100 ms lease.
* {{expiredLeaseCallbackRunsAfterShutdownReturns}}: the monitor paused inside
{{release}} of an expired lease until {{shutdown}} interrupts it. The callback
ran after {{shutdown}} returned.
SCM was not run; the masking in SCM is from reading the code.
h3. Patch
[^MC-3-lease-manager-shutdown-excludes-acquire-and-waits-for-callbacks.patch],
against f804bf26867246067017fdd9e5319c2cbdbd5db8. It also applies to master at
a7da4f70195.
{{shutdown}} now:
# joins the monitor thread, bounded and without the manager lock, so the
monitor finishes its current pass while the manager is still running (any lease
already past its deadline that the pass reaches is expired and gets its
callback);
# runs the release loop and {{isRunning = false}} in one {{synchronized}}
block, so a concurrent {{acquire}} either completes before it and is released,
or fails with {{LeaseManagerNotRunningException}};
# shuts down the callback executor with {{HadoopExecutors.shutdown}}, bounded
and outside the lock, so submitted callbacks complete before {{shutdown}}
returns.
{{isRunning}} becomes {{volatile}} for the unsynchronized readers. No wait
happens under the lock, because the monitor and callbacks may take it. A
callback whose expiry was decided before the shutdown therefore still runs, but
before {{shutdown}} returns; active leases are released without callbacks as
before. {{shutdown}} can now block for a bounded time (5 s for the monitor, up
to 10 s for callbacks); normally it returns at once. A callback that calls
{{shutdown}} itself waits out the 5 s bound and logs one ERROR, without a
deadlock. The wait for the manager lock is unbounded, as before. An interrupted
caller still waits within these bounds, and its interrupt flag is restored when
{{shutdown}} returns.
Covered by {{testShutdownReleasesLeaseAcquiredConcurrently}} and
{{testShutdownWaitsForCallbackOfExpiredLease}} (run for a normal and an
interrupted caller) in the existing {{TestLeaseManager}}. Without the change
all three cases fail ("lease acquired while shutdown() was running is released:
Expecting value to be true but was false" and "shutdown() returned while the
callback was still running"). With it {{TestLeaseManager}} (13 tests),
{{TestEventWatcher}}, {{TestBlockManager}} and
{{TestCloseContainerEventHandler}} pass, checkstyle is clean, and the attached
reproduction no longer shows either race.
Found by TLA+ model checking and code review of the HDDS LeaseManager expiry
and shutdown paths under HDDS-15926, on commit
f804bf26867246067017fdd9e5319c2cbdbd5db8. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. Related but not
duplicates: HDDS-2561 (interrupt handling in the lease monitor), HDDS-8418
(callback registration after expiry) and HDDS-4526 (false positive error logs
from LeaseManager). 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]
