Skip to content

fix: shutdown on uncaught exception #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aaron-steinfeld merged 1 commit into from
Jul 31, 2023
Merged

fix: shutdown on uncaught exception #73

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions ...service-framework/src/main/java/org/hypertrace/core/serviceframework/PlatformService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,12 @@ public void start() {
context.addServlet(new ServletHolder(new JVMDiagnosticServlet()), "/diags/*");

final Thread thread = new Thread(this::doStart);
thread.setUncaughtExceptionHandler(
Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The potential risk here is that there may be some exceptions that are ignorable today and uncaught that would lead to a server quit. IMO we should be fixing any such spots (And kube will recover for us in the interim)

(threadWithException, exception) -> this.shutdownWithError(exception));
try {
thread.start();
} catch (Exception e) {
LOGGER.error("Failed to start thread for application.", e);
System.exit(1);
throw e;
this.shutdownWithError(e);
}

// Start the webserver.
Expand All @@ -152,9 +152,7 @@ public void start() {
thread.join();
adminServer.join();
} catch (Exception e) {
LOGGER.error("Failed to start service servlet.");
this.shutdown();
System.exit(1);
this.shutdownWithError(e);
}
}

Expand Down Expand Up @@ -190,4 +188,14 @@ public void shutdown() {
PlatformMetricsRegistry.stop();
LOGGER.info("Service - {} is shutdown.", getServiceName());
}

private void shutdownWithError(Throwable exception) {
LOGGER.error("Shutting down due to unrecoverable exception", exception);
try {
this.shutdown();
} catch (Exception e) {
// Ignore if failed to shut down
Copy link
Contributor

@tim-mwangi tim-mwangi Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this happen?

Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially. The shutdown method calls various shutdown hooks, which each service is able to define itself. If any happen to throw (which seems like it's possible especially given it can now be called before startup), we don't want them to actually prevent the shutdown from completing.

Copy link
Contributor

@tim-mwangi tim-mwangi Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log or it would be too noisy?

Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a noise volume concern, i was just concerned it would be misleading. Basically something has happened to cause the server to shutdown abnormally. If we hit more errors when trying to force that shutdown (from a server that may not have started in the first place), we don't want those to distract from the root cause.

}
System.exit(1);
}
}