smengcl commented on code in PR #8805:
URL: https://github.com/apache/hadoop/pull/8805#discussion_r4279439661
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -943,8 +944,15 @@ public void run() {
}
}
} catch (Throwable t) {
+ if (!running) {
+ // stopThreads() interrupts this thread, which can surface as an
+ // exception from the backing store; don't kill the process for it.
+ LOG.warn("ExpiredTokenRemover thread received exception while"
+ + " stopping", t);
+ return;
+ }
LOG.error("ExpiredTokenRemover thread received unexpected exception",
t);
- Runtime.getRuntime().exit(-1);
+ ExitUtil.terminate(-1, t);
Review Comment:
Not for this PR: `SecurityUtil.doAsLoginUserOrFatal` is the only other
non-test caller of `Runtime.getRuntime().exit(-1)` in hadoop-common. Could be a
follow-up JIRA.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -943,8 +944,15 @@ public void run() {
}
}
} catch (Throwable t) {
+ if (!running) {
Review Comment:
This also swallows `Error`s (e.g. OOM) raised during shutdown. Would `if
(!running && !(t instanceof Error))` be safer?
Also, a full WARN stack trace on an otherwise normal stop may be noisy. INFO
with `t.toString()` might be enough.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestDelegationToken.java:
##########
@@ -425,6 +430,78 @@ public Object run() throws Exception {
}
}
+ /**
+ * Returns a secret manager that runs onFail on every incrementCurrentKeyId()
+ * call after the first. The first call comes from startThreads(); the next
+ * one comes from rollMasterKey() in the remover thread.
+ */
+ private static TestDelegationTokenSecretManager failingAfterFirstKeyId(
+ Runnable onFail) {
+ AtomicInteger keyIdCalls = new AtomicInteger();
+ return new TestDelegationTokenSecretManager(3600000, 3600000, 3600000,
+ 3600000) {
+ @Override
+ protected int incrementCurrentKeyId() {
+ if (keyIdCalls.incrementAndGet() > 1) {
+ onFail.run();
+ }
+ return super.incrementCurrentKeyId();
+ }
+ };
+ }
+
+ /**
+ * An unexpected exception in the expired token remover thread must go
+ * through ExitUtil so that tests can intercept it.
+ */
+ @Test
+ @Timeout(value = 30)
+ public void testExpiredTokenRemoverFailureTerminates() throws Exception {
+ ExitUtil.disableSystemExit();
+ TestDelegationTokenSecretManager dtSecretManager =
+ failingAfterFirstKeyId(() -> {
+ throw new RuntimeException("injected key id failure");
+ });
+ try {
+ dtSecretManager.startThreads();
+ GenericTestUtils.waitFor(ExitUtil::terminateCalled, 100, 10000);
+ ExitUtil.ExitException ee = ExitUtil.getFirstExitException();
+ assertEquals(-1, ee.getExitCode());
+ assertEquals("injected key id failure", ee.getCause().getMessage());
Review Comment:
nit: `assertThat(ee.getCause()).hasMessage("injected key id failure")` reads
a bit better.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java:
##########
@@ -96,6 +101,11 @@ public void tearDown() throws Exception {
if (zkServer != null) {
zkServer.close();
}
+ ExitUtil.ExitException ee = ExitUtil.getFirstExitException();
+ ExitUtil.resetFirstExitException();
+ if (ee != null) {
+ throw new AssertionError("ExitUtil.terminate was called", ee);
Review Comment:
nit: `fail("ExitUtil.terminate was called", ee)`
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java:
##########
@@ -96,6 +101,11 @@ public void tearDown() throws Exception {
if (zkServer != null) {
zkServer.close();
}
+ ExitUtil.ExitException ee = ExitUtil.getFirstExitException();
Review Comment:
If `zkServer.close()` throws, this check and the reset are skipped, so the
exit state leaks into the next test. Maybe move the ExitUtil check into a
`finally`.
Also worth a note in the comment: a leaked remover that terminates after
this point is reported by the next test's `tearDown()`.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestDelegationToken.java:
##########
@@ -425,6 +430,78 @@ public Object run() throws Exception {
}
}
+ /**
+ * Returns a secret manager that runs onFail on every incrementCurrentKeyId()
+ * call after the first. The first call comes from startThreads(); the next
+ * one comes from rollMasterKey() in the remover thread.
+ */
+ private static TestDelegationTokenSecretManager failingAfterFirstKeyId(
+ Runnable onFail) {
+ AtomicInteger keyIdCalls = new AtomicInteger();
+ return new TestDelegationTokenSecretManager(3600000, 3600000, 3600000,
+ 3600000) {
+ @Override
+ protected int incrementCurrentKeyId() {
+ if (keyIdCalls.incrementAndGet() > 1) {
+ onFail.run();
+ }
+ return super.incrementCurrentKeyId();
+ }
+ };
+ }
+
+ /**
+ * An unexpected exception in the expired token remover thread must go
+ * through ExitUtil so that tests can intercept it.
+ */
+ @Test
+ @Timeout(value = 30)
+ public void testExpiredTokenRemoverFailureTerminates() throws Exception {
+ ExitUtil.disableSystemExit();
+ TestDelegationTokenSecretManager dtSecretManager =
+ failingAfterFirstKeyId(() -> {
+ throw new RuntimeException("injected key id failure");
+ });
+ try {
+ dtSecretManager.startThreads();
+ GenericTestUtils.waitFor(ExitUtil::terminateCalled, 100, 10000);
+ ExitUtil.ExitException ee = ExitUtil.getFirstExitException();
+ assertEquals(-1, ee.getExitCode());
+ assertEquals("injected key id failure", ee.getCause().getMessage());
+ } finally {
+ dtSecretManager.stopThreads();
+ ExitUtil.resetFirstExitException();
+ }
+ }
+
+ /**
+ * An exception raised because stopThreads() interrupted the expired token
+ * remover thread must not terminate the process.
+ */
+ @Test
+ @Timeout(value = 30)
+ public void testExpiredTokenRemoverFailureWhileStoppingDoesNotTerminate()
+ throws Exception {
+ ExitUtil.disableSystemExit();
+ CountDownLatch blocked = new CountDownLatch(1);
+ TestDelegationTokenSecretManager dtSecretManager =
+ failingAfterFirstKeyId(() -> {
+ // Block in the remover thread until stopThreads() interrupts it,
+ // as a backing store call would.
+ blocked.countDown();
+ try {
+ new CountDownLatch(1).await();
+ } catch (InterruptedException e) {
+ throw new RuntimeException("interrupted", e);
+ }
+ });
+ dtSecretManager.startThreads();
+ assertTrue(blocked.await(10, TimeUnit.SECONDS));
+ dtSecretManager.stopThreads();
+ assertFalse(ExitUtil.terminateCalled(),
Review Comment:
Unlike the test above, there is no cleanup here. If this assertion fails,
the recorded exit is never reset. If the latch wait times out, `stopThreads()`
is skipped and the remover stays blocked. Could we stop the manager and call
`ExitUtil.resetFirstExitException()` in a `finally`, or in an `@AfterEach`
shared by both tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
