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