Skip to content
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

Jump to bottom

fix: abort transaction in all cases #881

Merged
merged 1 commit into from Apr 13, 2022
Merged

fix: abort transaction in all cases #881

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion driver-sync/src/main/com/mongodb/client/internal/ClientSessionImpl.java
View file
Open in desktop
Expand Up @@ -209,7 +209,7 @@ public <T> T withTransaction(final TransactionBody<T> transactionBody, final Tra
try {
startTransaction(options);
retVal = transactionBody.execute();
} catch (RuntimeException e) {
} catch (Throwable e) {
Copy link
Member

@stIncMale stIncMale Apr 12, 2022 •
edited

Choose a reason for hiding this comment

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

Such a tiny change, and so many things come to mind when thinking about it.

Errors in Java the way they are, make no sense

My view on this is expressed here. A consequence of this view is that we should not catch Errors, which means we should catch Exceptions instead of catching Throwables each time we want to catch "everything" that makes sense catching (surely, there are exceptions from this rule, like catching assertion errors in test frameworks, or catching specific Errors that should have been Exceptions, but were made Errors instead).

Unlike Java, Rust uses two drastically different mechanisms to represent recoverable and unrecoverable errors:

  • Recoverable errors are represented via return values. Unlike in C, the compiler forces a caller to either handle them or to explicitly propagate as the caller's return value via the ? operator.
  • Unrecoverable errors are called panics, which can be implemented via unwinding or aborting. Unwinding is similar to throwing an exception in Java, and an unwinding panic can even be caught.

It is worth pointing out that the ability to catch unwinding panics in Rust should not be interpreted as "see, even in Rust catching unrecoverable errors is possible, which suggests that trying to handle unrecoverable errors (whether in Java or in Rust) may make sense!". The reason Rust allows catching unwinding panics is that "Rust's unwinding strategy is not specified to be fundamentally compatible with any other language's unwinding. As such, unwinding into Rust from another language, or unwinding into another language from Rust is Undefined Behavior. You must absolutely catch any panics at the FFI boundary!" This point leads us directly to the issue that this PR is trying to address.

Kotlin's exception handling is not compatible with Java's exception handling

Unlike Java, Kotlin does not have the concept of checked exceptions (good for Kotlin). However, this means that Kotlin's exception mechanism is not compatible with Java's exception mechanism, and just like Rust code must catch any unwinding panics at the FFI boundary, Kotlin generally should catch all exceptions that are considered checked in Java (the check is simple: they are of the Exception type and not of the RuntimeException type) at the Kotlin-Java boundary. Unfortunately, the Kotlin's interop guide erroneously does not require anything like this. It partially addresses the incompatibility for a subset of cases, but that subset does not include the case addressed in this PR.

Conclusion

  1. Even though I consider catching Throwables instead of Exceptions an undesirable practice, the code of the driver contains many places where this is done, which is why introducing one more such catch changes nothing.
  2. Even though having unchecked exception thrown from Kotlin code that implements a Java API method that does not declare checked exceptions is a problem in the Kotlin code, we better work around it as suggested in the PR. Also, from now on we should remember to consider any method as being able to throw checked exception even it is not declared1 (despite this not being true for all methods, for simplicity and robustness it is better to treat all of them this way).

1 One may argue that even without calling Kotlin code, sneaky throws are possible in Java due to the bug-feature it introduced. However, in this case it is easy to argue that whoever uses the technique is fully responsible for the consequences. The same is much harder to say about Kotlin code because it is more abundant and Kotlin's documentation fails to require the right thing to be done for interoperability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice reasoning. I add that if you decompile a Kotlin class file you see that a checked exception is wrapped inside a Throwable and then thrown, I've not tested it but I'd guess that it's done in Scala and Groovy too.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, it is not obvious to me why Kotlin compiler wraps checked exceptions in Throwable given that an exception of the Throwable type is considered checked by Java compiler.

if (transactionState == TransactionState.IN) {
abortTransaction();
}
Expand Down