[
https://issues.apache.org/jira/browse/HADOOP-20010?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18127330#comment-18127330
]
ASF GitHub Bot commented on HADOOP-20010:
-----------------------------------------
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?
> ExpiredTokenRemover should use ExitUtil instead of Runtime.exit
> ---------------------------------------------------------------
>
> Key: HADOOP-20010
> URL: https://issues.apache.org/jira/browse/HADOOP-20010
> Project: Hadoop Common
> Issue Type: Bug
> Components: security
> Reporter: Jose Luis López
> Assignee: Jose Luis López
> Priority: Major
> Labels: pull-request-available
>
> {{AbstractDelegationTokenSecretManager.ExpiredTokenRemover.run()}} catches
> every {{Throwable}} and calls {{Runtime.getRuntime().exit(-1)}}. This has two
> problems:
> # *It bypasses {{ExitUtil}}.* Tests that call
> {{ExitUtil.disableSystemExit()}} can't intercept it, so the JVM is killed
> instead. In {{TestZKDelegationTokenSecretManager}} and
> {{TestZKDelegationTokenSecretManagerImpl}}, a token manager leaked by a
> failing test keeps its remover thread running after {{tearDown()}} closes the
> ZooKeeper {{TestingServer}}. On its next pass the thread hits
> {{ConnectionLoss}}, which is wrapped in a {{RuntimeException}}, and kills the
> surefire fork. The original test failure is lost.
> # *It exits during a normal shutdown.* {{stopThreads()}} sets {{running =
> false}} and then interrupts the thread. With a ZooKeeper-backed store, the
> interrupt can surface as a {{RuntimeException}} instead of an
> {{InterruptedException}}, and that terminates the process while it is being
> stopped.
> *Proposed fix:*
> * Call {{ExitUtil.terminate(-1, t)}} instead of
> {{Runtime.getRuntime().exit(-1)}}.
> * If {{running}} is already false, log the exception and return instead of
> exiting.
> * Make {{TestZKDelegationTokenSecretManager}} disable system exit and fail in
> {{tearDown()}} if {{ExitUtil.terminate}} was called.
> *Tests:* two new tests in {{TestDelegationToken}}:
> * A remover failure while running calls {{ExitUtil.terminate(-1, ...)}}.
> * A failure while {{stopThreads()}} is running does not.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
