[
https://issues.apache.org/jira/browse/HDDS-17403?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17403:
------------------------------
Attachment:
MC-3-lease-manager-shutdown-excludes-acquire-and-waits-for-callbacks.patch
> 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
> Affects Versions: 0.2.1, 0.3.0, 0.4.0, 0.4.1, 0.5.0, 1.0.0, 1.1.0, 1.2.0,
> 1.3.0, 1.2.1, 1.4.0, 2.0.0, 1.4.1, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1
> Reporter: Siyao Meng
> Priority: Minor
> 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]
