typedb icon indicating copy to clipboard operation
typedb copied to clipboard

TypeDBException should not be a RuntimeException we catch

Open flyingsilverfin opened this issue 4 years ago • 1 comments

Problem to Solve

Currently, we use GraknException, which is a RuntimeException, and catch it in the QueryManager, which is a strange behaviour. We should only be catching CheckedExceptions.

This issue is from a TODO in GraknCheckedException.

Current Design

The current design requires that all places throwing exceptions in //concept use the GraphManager.exception() method to close the transaction. This is because a user could be going via RPC to the Concept API (or when using Grakn embedded - eg tests) and an error should kill the transaction. In lower level packages, we throw runtime exceptions GraknException which are expected to be caught in the other layer the user may interact with Grakn - the QueryManager.

This design that protects at two layers (//concept and QueryManager) requires us to catch RuntimeExceptions, which is a code smell. It also means that calls that go through QueryManager and //concept will close the transaction twice which, as transaction.close() is idempotent, is technically okay but a bit odd.

Proposed Design

The "correct" design would be to use GraknCheckedException everywhere that is supposed to cause the transaction to close. However, this pollutes almost all method signatures.

A middle ground could be this: every exception that should cause the transaction to close, must go through the GraphManager.exception() method (to clean up resources), and then may throw a RuntimeException that reports the tack trace for debugging.

However, not all classes have members that have access to GraphManager.exception() (eg. in ConceptManager or GraphManager). Examples of this are Encoding, any interface, IIDclasses, etc. An exception in these classes should _still_ force the transaction to close. This is the correct place to use a realGraknCheckedException, which the caller should catch and handle by passing it through to GraphManager.exception()` directly or indirectly.

The risk here is that we forget to throw an exception via the GraphManager.exception() and fail to close resources sometimes.

flyingsilverfin avatar Jan 13 '21 10:01 flyingsilverfin

Deprioritised into Backlog, @flyingsilverfin. Also, don't put backticks in the title. Titles should be clean, simple and short

haikalpribadi avatar Jan 14 '21 13:01 haikalpribadi