[
https://issues.apache.org/jira/browse/HDDS-17402?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17402:
------------------------------
Attachment: CR-1-keep-lease-monitor-running-after-unexpected-error.patch
> LeaseManager stops expiring leases for good after one unexpected error in its
> monitor thread, because the restart calls start() on the dead thread
> --------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17402
> URL: https://issues.apache.org/jira/browse/HDDS-17402
> 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:
> CR-1-keep-lease-monitor-running-after-unexpected-error.patch,
> TestBugCR1LeaseMonitorDeath.java
>
>
> h3. Mechanism
> {{LeaseManager}} expires leases on a single {{LeaseMonitor}} thread. The loop
> in {{LeaseMonitor.run}} only catches {{LeaseNotFoundException}} and
> {{LeaseExpiredException}}, so any other Throwable ends the thread. The
> uncaught exception handler installed in {{LeaseManager.start}} logs the error
> and calls {{leaseMonitorThread.start()}} on the same {{Thread}} object, which
> always throws {{IllegalThreadStateException}}. The JVM only prints it to
> stderr, and no thread is left to expire leases. The handler comment ("Let us
> just restart this thread after logging an error") shows the restart was meant
> to keep expiry alive; it can never work.
> The lease being expired at that moment also loses its callback when the
> failure comes from {{executorService.execute}}: {{release(resource)}} has
> already removed and invalidated it one line earlier.
> h3. Trigger
> On a running {{LeaseManager}} the loop body cannot throw a
> {{RuntimeException}} by itself: the {{Lease}} getters only throw the checked
> {{LeaseExpiredException}}, {{get}} and {{release}} only throw
> {{LeaseManagerNotRunningException}} after {{shutdown()}}, callbacks run on
> the pool and their exceptions are caught there, and the cached pool is never
> shut down. What remains is a JVM {{Error}}, in practice {{OutOfMemoryError:
> unable to create native thread}} from {{executorService.execute}} when the
> process hits its thread limit, or an exception from the
> {{hashCode}}/{{equals}} of a caller supplied resource. SCM's resource
> ({{CloseContainerCommand}}) cannot throw there, so in SCM it takes thread
> exhaustion caused elsewhere in the process.
> h3. Impact
> * From the failure on, no lease expires and no callback runs for the life of
> the process. Every timed out resource stays in {{activeLeases}} and acquiring
> it again throws {{LeaseAlreadyExistException}} (both run, see Reproduction).
> * In SCM, {{CloseContainerEventHandler}} uses the lease to delay the close
> command for a CLOSING container. After the monitor dies those delayed close
> commands are never sent, repeat close events only log
> {{LeaseAlreadyExistException}} at DEBUG, and {{activeLeases}} grows by one
> entry per closed container. ReplicationManager still sends close commands to
> the replicas of CLOSING containers on each pass
> ({{hdds.scm.replication.thread.interval}}, 300s by default), so containers
> still close, later. This SCM part is argued from the code, not run on a
> cluster.
> * An outside interrupt of the monitor while it is enabled would make the loop
> spin, but no code path does that: the thread is private and the only
> {{interrupt()}} is in {{shutdown()}}, after the loop is disabled.
> h3. Reproduction
> [^TestBugCR1LeaseMonitorDeath.java] ({{hadoop-hdds/framework}}, package
> {{org.apache.hadoop.ozone.lease}}), deterministic, unmodified source. A lease
> resource whose {{hashCode}} throws {{OutOfMemoryError("unable to create
> native thread")}} the first time the monitor thread looks it up stands in for
> the thread exhaustion. The test shows the monitor thread TERMINATED, the
> stderr line {{Exception: java.lang.IllegalThreadStateException thrown from
> the UncaughtExceptionHandler}}, a later 100 ms lease whose callback does not
> run within 2 s and whose resource cannot be acquired again, and that
> {{start()}} on the dead thread throws {{IllegalThreadStateException}}. The
> private thread field is read by reflection for observation only. A passing
> test means the defect is present.
> The real trigger was also run outside the test suite: lease callbacks that
> block until released, until {{executorService.execute}} on the monitor thread
> failed with {{OutOfMemoryError: unable to create native thread}} at the per
> process thread limit. On the unmodified code the monitor died with the same
> {{IllegalThreadStateException}}, and after the pressure was removed a 100 ms
> lease never expired. With the patch the same run logged each failed
> submission, the monitor stayed alive, and the probe lease expired and could
> be acquired again.
> h3. Patch
> [^CR-1-keep-lease-monitor-running-after-unexpected-error.patch], against
> f804bf26867246067017fdd9e5319c2cbdbd5db8. It also applies to master at
> a7da4f70195.
> {{LeaseMonitor.run}} now catches any other Throwable per lease, logs it at
> ERROR with the manager name and the resource, and continues with the next
> lease, so one failure cannot stop expiry for all of them. It also bounds the
> next wait to the default lease timeout, but at least one second, so a lease
> whose check failed before {{release}} is retried by the monitor itself even
> if no other acquire or expiry wakes it, and a persistent failure logs at most
> once per that interval instead of spinning. A failed callback submission
> after {{release}} is logged, and that one callback still does not run.
> Catching {{Throwable}} rather than only {{RuntimeException}} is deliberate:
> the realistic trigger is an {{Error}}, and surviving it is what the existing
> handler already tried to do. The uncaught exception handler no longer
> pretends to restart the thread; it only logs. Rebuilding a new thread in the
> handler was not chosen: it needs more code in {{start()}}, must not restart
> after {{shutdown()}}, restarts in a tight loop on a persistent failure, and
> still loses the lease in flight.
> Covered by {{testLeaseMonitorSurvivesUnexpectedError}} in the existing
> {{TestLeaseManager}}: the monitor's first lookup of the only lease's resource
> throws an {{Error}}, and that lease must still expire and run its callback
> with nothing else waking the monitor. It times out on the unmodified code
> (dead monitor) and also with the catch but without the bounded wait. With the
> patch {{TestLeaseManager}}, {{TestEventWatcher}},
> {{TestCloseContainerEventHandler}} and {{TestBlockManager}} (31 tests) pass
> and checkstyle is clean. With the patch the reproduction fails at its first
> assertion, because the monitor thread is still alive.
> Found by code review of the HDDS LeaseManager expiry and shutdown paths, as
> part of the TLA+ verification effort under HDDS-15926, on commit
> f804bf26867246067017fdd9e5319c2cbdbd5db8. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. 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]
