-
Notifications
You must be signed in to change notification settings - Fork 290
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.
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
supporting KJ_CLEAN_SHUTDOWN in the server #2152
Conversation
I actually don't think this is a right fix: this won't tear down the CliMain object that holds all the isolate references. Need to tweak it more. |
930f3ce
to
9109fb6
Compare
9109fb6
to
c63dcbe
Compare
I redid the fix. PTAL. @jasnell |
@@ -1179,7 +1179,7 @@ public: | |||
} | |||
|
|||
template <typename Func> | |||
[[noreturn]] void serveImpl(Func&& func) noexcept { | |||
void serveImpl(Func&& func) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you verify that uncaught exceptions thrown from here are still reported in a reasonable way? Usually the reason I declare functions noexcept
is specifically to force calling the termination handler directly without unwinding if an exception is thrown. It's possible that removing it leads to the exception being caught and reported in a worse way somewhere up-stack and/or triggering other bugs during unwind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this:
$ git diff
diff --git a/src/workerd/server/workerd.c++ b/src/workerd/server/workerd.c++
index 63cb372f..59543599 100644
--- a/src/workerd/server/workerd.c++
+++ b/src/workerd/server/workerd.c++
@@ -1180,6 +1180,7 @@ public:
template <typename Func>
void serveImpl(Func&& func) {
+ throw KJ_EXCEPTION(FAILED, "test exception");
if (hadErrors) {
// Can't start, stuff is broken.
KJ_IF_SOME(w, watcher) {
It results in:
$ bazel run //src/workerd/server:workerd -- serve $(pwd)/samples/helloworld/config.capnp
INFO: Invocation ID: 6e38988b-8f68-4ee6-a4c8-10d1f46e3ccd
INFO: Analyzed target //src/workerd/server:workerd (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
INFO: Writing explanation of rebuilds to '/tmp/bazel-explain'
Target //src/workerd/server:workerd up-to-date:
bazel-bin/src/workerd/server/workerd
INFO: Elapsed time: 15.053s, Critical Path: 14.81s
INFO: 3 processes: 1 internal, 2 linux-sandbox.
INFO: Build completed successfully, 3 total actions
INFO: Running command line: bazel-bin/src/workerd/server/workerd serve /home/maizatskyi/src/github.com/cloudflare/workerd/samples/helloworld/config.capnp
*** Uncaught exception ***
workerd/server/workerd.c++:1183: failed: test exception
Is this reasonable enough? Do you want me to try anything else?
Usually the reason I declare functions noexcept is specifically to force calling the termination handler directly without unwinding if an exception is thrown.
Agree, but in this case it was a straight up lie to compiler. Maybe there's a way to do clean shutdown in KJ with keeping noexcept
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this reasonable enough?
So, the *** Uncaught exception ***
comes from runMainAndExit()
in kj/main.c++
where it actually catches the exception and reports it. This is subtly different from terminate being called in that when an exception is thrown and then caught, stack unwinding occurs in between, but when an exception is thrown somewhere where it will trigger termination, then the terminate handler is called immediately without unwinding the stack. This has a couple implications:
- A destructor could mess up and crash us in a different way before we get a chance to see the error message. This is actually pretty common when unwinding top-level objects whose teardown is not well-tested, combined with the exception probably indicating we're in a weird state during teardown.
- The terminate handler can produce a stack trace regardless of the type of exception since the stack is still live, whereas when we catch an exception we can only report a stack trace if it is a
kj::Exception
and therefore includes one.
So basically, the error reporting is a little bit worse now.
in this case it was a straight up lie to compiler.
I don't agree. noexcept
doesn't mean "this code will not throw", it means "if this code throws it should be promoted to a crash".
Maybe there's a way to do clean shutdown in KJ with keeping noexcept?
Yes, when clean shutdown is desired, instead of calling context.exit()
at the end of the function, just return.
No description provided.