From 41f4f2ef3c35133f5e0793387c87f9470e326218 Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Tue, 19 Aug 2025 15:24:11 +0000 Subject: [PATCH 1/6] avoid loss of error thrown from server run() Follow up to #5796 that attempts to handle the following case. 1. server.runServer() throws an error 2. server.close() throws en exception When the above happens the error will never be logged and its also not attached to the exception thrown in the finally block by server.close(). Therefore the error is lost and there is no indication it happeneded. With these changes the exception thrown by server.close() will be attached to the error thrown by server.runServer(). --- .../accumulo/server/AbstractServer.java | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java index 93cdf7a3af3..963891e18a9 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java +++ b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java @@ -76,15 +76,7 @@ public static interface ThriftServerSupplier { } public static void startServer(AbstractServer server, Logger LOG) throws Exception { - try { - server.runServer(); - } catch (Exception e) { - System.err - .println(server.getClass().getSimpleName() + " died, exception thrown from runServer."); - e.printStackTrace(); - LOG.error("{} died, exception thrown from runServer.", server.getClass().getSimpleName(), e); - throw e; - } finally { + AutoCloseable ac = () -> { try { server.close(); } catch (Exception e) { @@ -93,6 +85,16 @@ public static void startServer(AbstractServer server, Logger LOG) throws Excepti LOG.error("Exception thrown while closing {}", server.getClass().getSimpleName(), e); throw e; } + }; + + try (var unused = ac) { + server.runServer(); + } catch (Exception e) { + System.err + .println(server.getClass().getSimpleName() + " died, exception thrown from runServer."); + e.printStackTrace(); + LOG.error("{} died, exception thrown from runServer.", server.getClass().getSimpleName(), e); + throw e; } } From 0aa9a7481433353ac68f77bc18311d9d78b13ca0 Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Tue, 19 Aug 2025 15:52:28 +0000 Subject: [PATCH 2/6] add unit test --- .../accumulo/server/AbstractServerTest.java | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 server/base/src/test/java/org/apache/accumulo/server/AbstractServerTest.java diff --git a/server/base/src/test/java/org/apache/accumulo/server/AbstractServerTest.java b/server/base/src/test/java/org/apache/accumulo/server/AbstractServerTest.java new file mode 100644 index 00000000000..ba8f2de167f --- /dev/null +++ b/server/base/src/test/java/org/apache/accumulo/server/AbstractServerTest.java @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.server; + +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.mock; +import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.verify; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; + +public class AbstractServerTest { + @Test + public void testError() throws Exception { + AbstractServer server = mock(AbstractServer.class); + server.runServer(); + expectLastCall().andThrow(new UnknownError()); + server.close(); + expectLastCall().andThrow(new IllegalStateException()); + + replay(server); + var error = assertThrows(UnknownError.class, () -> AbstractServer.startServer(server, + LoggerFactory.getLogger(AbstractServerTest.class))); + assertEquals(1, error.getSuppressed().length); + assertEquals(IllegalStateException.class, error.getSuppressed()[0].getClass()); + verify(server); + } +} From 6047997c48d9b6886105674d92598fa4e2577308 Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Tue, 19 Aug 2025 12:34:55 -0400 Subject: [PATCH 3/6] Update server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java Co-authored-by: Dave Marion --- .../main/java/org/apache/accumulo/server/AbstractServer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java index 963891e18a9..ef440b147db 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java +++ b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java @@ -79,7 +79,7 @@ public static void startServer(AbstractServer server, Logger LOG) throws Excepti AutoCloseable ac = () -> { try { server.close(); - } catch (Exception e) { + } catch (Throwable e) { System.err.println("Exception thrown while closing " + server.getClass().getSimpleName()); e.printStackTrace(); LOG.error("Exception thrown while closing {}", server.getClass().getSimpleName(), e); From 421d09176e315c78e5903e265973afe671994be2 Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Tue, 19 Aug 2025 12:35:02 -0400 Subject: [PATCH 4/6] Update server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java Co-authored-by: Dave Marion --- .../main/java/org/apache/accumulo/server/AbstractServer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java index ef440b147db..ed70a32ed40 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java +++ b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java @@ -89,7 +89,7 @@ public static void startServer(AbstractServer server, Logger LOG) throws Excepti try (var unused = ac) { server.runServer(); - } catch (Exception e) { + } catch (Throwable e) { System.err .println(server.getClass().getSimpleName() + " died, exception thrown from runServer."); e.printStackTrace(); From 4a0fe5f4320e6e149a703d2a3c4c449034ddc2ad Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Tue, 19 Aug 2025 16:57:15 +0000 Subject: [PATCH 5/6] revert changes and only catch throwable --- .../accumulo/server/AbstractServer.java | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java index ed70a32ed40..e6543afe17d 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java +++ b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java @@ -76,7 +76,15 @@ public static interface ThriftServerSupplier { } public static void startServer(AbstractServer server, Logger LOG) throws Exception { - AutoCloseable ac = () -> { + try { + server.runServer(); + } catch (Throwable e) { + System.err + .println(server.getClass().getSimpleName() + " died, exception thrown from runServer."); + e.printStackTrace(); + LOG.error("{} died, exception thrown from runServer.", server.getClass().getSimpleName(), e); + throw e; + } finally { try { server.close(); } catch (Throwable e) { @@ -85,16 +93,6 @@ public static void startServer(AbstractServer server, Logger LOG) throws Excepti LOG.error("Exception thrown while closing {}", server.getClass().getSimpleName(), e); throw e; } - }; - - try (var unused = ac) { - server.runServer(); - } catch (Throwable e) { - System.err - .println(server.getClass().getSimpleName() + " died, exception thrown from runServer."); - e.printStackTrace(); - LOG.error("{} died, exception thrown from runServer.", server.getClass().getSimpleName(), e); - throw e; } } From 7bd4eb250c47dace371eef4b1e351623e6fd8683 Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Tue, 19 Aug 2025 17:06:55 +0000 Subject: [PATCH 6/6] remove test --- .../accumulo/server/AbstractServerTest.java | 47 ------------------- 1 file changed, 47 deletions(-) delete mode 100644 server/base/src/test/java/org/apache/accumulo/server/AbstractServerTest.java diff --git a/server/base/src/test/java/org/apache/accumulo/server/AbstractServerTest.java b/server/base/src/test/java/org/apache/accumulo/server/AbstractServerTest.java deleted file mode 100644 index ba8f2de167f..00000000000 --- a/server/base/src/test/java/org/apache/accumulo/server/AbstractServerTest.java +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.accumulo.server; - -import static org.easymock.EasyMock.expectLastCall; -import static org.easymock.EasyMock.mock; -import static org.easymock.EasyMock.replay; -import static org.easymock.EasyMock.verify; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; - -import org.junit.jupiter.api.Test; -import org.slf4j.LoggerFactory; - -public class AbstractServerTest { - @Test - public void testError() throws Exception { - AbstractServer server = mock(AbstractServer.class); - server.runServer(); - expectLastCall().andThrow(new UnknownError()); - server.close(); - expectLastCall().andThrow(new IllegalStateException()); - - replay(server); - var error = assertThrows(UnknownError.class, () -> AbstractServer.startServer(server, - LoggerFactory.getLogger(AbstractServerTest.class))); - assertEquals(1, error.getSuppressed().length); - assertEquals(IllegalStateException.class, error.getSuppressed()[0].getClass()); - verify(server); - } -}