Skip to content

Conversation

@harrishancock
Copy link
Collaborator

This PR contains a proof of concept macro which allows us to write code like:

JSG_TRY {
  // code that might throw either a JS or KJ exception
} catch (...) {
  jsg::Value exception = getCaughtExceptionAsJsg();
  // code that handles the exception
}

In the snippet above, getCaughtExceptionAsJsg is a function object which is declared in the JSG_TRY macro. The JSG_TRY macro uses the same trick that KJ_IF_SOME pioneered to make a declaration available to only one single statement without requiring extra braces. getCaughtExceptionAsJsg(), when called, performs the same exception conversion that jsg::Lock::tryCatch() performs. Instead of passing the resulting value to a callback, however, it returns it.

Some questions you might be asking:

Q: Does getCaughtExceptionAsJsg() remember to destroy the relevant v8::TryCatch object before returning?
A: Yes. It holds it in a Maybe, and nullifies the Maybe.

Q: Can I name getCaughtExceptionAsJsg() something else?
A: Yes. I included a JSG_TRY_CATCH macro to show how that might look. I like that version better, in fact, but it's more verbose.

Q: If it uses the same trick as KJ_IF_SOME, does that mean this thing is actually an if statement?
A: Yes, but it is a complete if-else statement, which avoids some pitfalls. KJ_IF_SOME expands to if (decl1) if (decl2; false) {} else, whereas JSG_TRY expands to the simpler if (decl; false) {} else try. The else already binds to the if, so it is a compile error to add another else, and constructs like if JSG_TRY { ... } catch (...) { ... } else ... work as you would expect.

Q: Does this produce any weird compiler warnings about dangling elses, like KJ_IF_SOME sometimes does?
A: I haven't seen any, but I'm not 100% sure.

Q: What's wrong with jsg::Lock::tryCatch()?
A: I want to implement co_await syntax for JSG promises, and that syntax is somewhat useless without real try-catch syntax to go with it. That said, this is not by itself sufficient for handling JS exceptions in coroutines -- we would need to arrange to destroy the v8::TryCatch on every suspension, and recreate it on every resumption.


This PR also includes a proof of concept set of macros, KJ_TRY and KJ_CATCH, which allow us to write code like:

KJ_TRY {
  // code that might throw any exception
} KJ_CATCH (kj::Exception& exception) {
  // code that handles the exception as a KJ exception
}

That is, it is syntax sugar for our normal catch (...) pattern:

try {
  // code that might throw any exception
} catch (...) {
  auto exception = kj::getCaughtExceptionAsKj();
  // code that handles the exception as a KJ exception
}

That said, I don't like it very much, because it uses a nested try-catch. I developed it because the JSG_TRY macro doubles down on our catch (...) pattern, so I wanted to explore if it was possible to avoid that. Turns out it is, but I'm not sure it's worth the cost.

I'm working on `co_await` syntax for jsg::Promises, and `co_await` syntax isn't particularly useful if you can't use it in a try block. But, JSG doesn't support raw try-catch blocks.

This is an experiment to see if we can fix that reasonably elegantly.

Claude helped me with the boring parts.
I was curious if we could get rid of the `catch (...) { auto e = kj::getExceptionAsKj()` pattern, since I doubled down on it in the JSG_TRY macro implementation.

Turns out we can. But I'm not sure that we should.

Claude again helped me with the boring parts.
kj::String testTryCatch2(Lock& js, jsg::Function<int()> thrower) {
// Here we prove that the macro is if-else friendly.
// Note that clang-format doesn't recognize it as a `try`, so we get wonky formatting. Oh well.
if (true) JSG_TRY {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An example of how JSG_TRY looks like in practice. Note that clang-format doesn't recognize JSG_TRY as a try keyword, so the formatting is messed up -- the catch is always formatted as if starting a new statement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm.. can clang-format be extended/updated to account for this I wonder?


void MessagePort::dispatchMessage(jsg::Lock& js, const jsg::JsValue& value) {
js.tryCatch([&] {
JSG_TRY_CATCH(getMessageException) try {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An example of how JSG_TRY_CATCH looks in practice. I like this better than JSG_TRY, because clang-format can see the full try-catch statement.

A version of JSG_TRY which includes the default name (getCaughtExceptionAsJsg) but doesn't include the try keyword could be a third option:

JSG_TRY try {
  ...
} catch (...) {
  jsg::Value exception = getCaughtExceptionAsJsg();
}

template <typename Func>
auto translateTeeErrors(Func&& f) -> decltype(kj::fwd<Func>(f)()) {
try {
KJ_TRY {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An example of KJ_TRY and KJ_CATCH at work.

Ultimately, I think I prefer the raw syntax, myself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed on prefering the raw syntax.


void DigestStream::dispose(jsg::Lock& js) {
js.tryCatch([&] {
JSG_TRY {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I appreciate that the structure is similar to try/catch syntax but the catch(...) there just feels.. odd.. for some reason. Feel free to ignore but given that the macro is a bit special I'd almost prefer something like...

JSG_TRY(
  {
    // Block to try
  },
  {
    // Catch block
  }
)

But even that feels bleh, lol. So feel free to ignore this 😆

Comment on lines +816 to +817
catch (...) {
jsg::Value exception = getCaughtExceptionAsJsg();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is likely to be a common pattern... perhaps even something like...

JSG_CATCH(exception) {
  js.throwException(kj::mv(exception));
}

To boilerplate this particular piece might be handy?

}
catch (...) {
jsg::Value exception = getCaughtExceptionAsJsg();
return js.rejectedPromise<void>(kj::mv(exception));
Copy link
Collaborator

Choose a reason for hiding this comment

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

there are going to be some common patterns like:

  1. Convert the error and throw it
  2. Convert the error and return a rejected promise

Maybe these can be boilerplated with their own macros?

JSG_CATCH_THROW()  // short cut for rethrowing
JSG_CATCH_RETURN_REJECTED()  // short cut for returning rejected

But also just a nit.. feel free to ignore.

// Handle any in-flight JsExceptionThrown or kj::Exception. If a JsExceptionThrown is found, our
// v8::TryCatch's currently held exception value is returned. If a kj::Exception is found, it is
// converted to a JS value by passing it and `options` to `Lock::exceptionToJs()`.
Value operator()(ExceptionToJsOptions options = {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a fairly non-trivial function in a non-templated class. Can it perhaps be moved into jsg.c++?

} catch (...) {
auto exception = kj::getCaughtExceptionAsKj();
}
KJ_CATCH(kj::Exception & exception) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this just be KJ_CATCH(exception) { ... the kj::Exception& part is likely a given.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants