typedb
typedb copied to clipboard
TypeDBException should not be a RuntimeException we catch
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 CheckedException
s.
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 real
GraknCheckedException, 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.
Deprioritised into Backlog, @flyingsilverfin. Also, don't put backticks in the title. Titles should be clean, simple and short