avoid loss of error thrown from server run()

[keith-turner]

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().

[dlmarion]

@keith-turner - in 1 above, are you making a distinction about Error vs Exception?

[keith-turner]

@keith-turner - in 1 above, are you making a distinction about Error vs Exception?

Yeah the current code will log for an exception where an error seems to be completely lost (no logging and its not thrown). I added a unit test in 0aa9a74 and I tried running that w/ the old and new changes to AbstractServer to see what the logging looks like.

[dlmarion]

Yeah, so we should probably just catch Throwable in both cases. I added a test concurrent to you, the following prints both, but doesn't link them using the suppression mechanism.

public class AbstractServerTest {
  
  private static final Logger LOG = LoggerFactory.getLogger(AbstractServerTest.class);

  public void throwError(String name) {
    throw new StackOverflowError(name);
  }

  public void throwException(String name) {
    throw new UnsupportedOperationException(name);
  }
  
  @Test
  public void testExceptions( ) {
    try {
      throwError("runServer");
    } catch (Throwable e) {
      System.err.println("died, exception thrown from runServer.");
      e.printStackTrace();
      LOG.error("{} died, exception thrown from runServer.", e);
      throw e;
    } finally {
      try {
        throwException("close");        
      } catch (Throwable e) {
        System.err.println("died, exception thrown from close.");  
        e.printStackTrace();
        LOG.error("{} died, exception thrown from close.", e);
        for (Throwable suppressed : e.getSuppressed()) {
          LOG.error("suppressed: ", suppressed);
        }
        throw e;
      }
    }
  }
}

[keith-turner]

Yeah, so we should probably just catch Throwable in both cases. I added a test concurrent to you, the following prints both, but doesn't link them using the suppression mechanism.

Not completely sure, but linking them may be useful because then the uncaught ecxception handler will see an error was thrown. Catching throwable would log the error, it just would not percolate out of the main method. Using try w/ resources it does percolate out of the main method, but then it does not log. Maybe need to do both?

[dlmarion]

Not completely sure, but linking them may be useful because then the uncaught ecxception handler will see an error was thrown.

Not sure if that matters in this case because if the runServer throws anything then the lock watcher is going to call halt and if close throws anything it's going to end the application anyway.

Catching throwable would log the error, it just would not percolate out of the main method. Using try w/ resources it does percolate out of the main method, but then it does not log. Maybe need to do both?

FYI that I was printing to stderr and logging. With asynchronous logging in log4j2 there is no guarantee that the logging will be flushed before the JVM is halted.

[keith-turner]

Looking at the output of running the unit test, it seems that close() is called before the error is printed. So that would invalidate what #5796 was trying to accomplish. Researching this a bit more to see what the order of execution is. If that is the behavior, may only be able to catch throwable and not use try w/ resources.

[keith-turner]

Looked into the behavior and confirmed that close() executed before any catch code. https://stackoverflow.com/a/25057349

That would invalidate what #5796 was trying to accomplish (the call to close() getting stuck before executing the catch code would prevent the logging), so removed the try w/ resource and only caught throwable. Can not easily link the exceptions in this case.