I think its a valid question. There is a convention in the Elixir standard library that functions which raise exceptions end with a bang, (such as Map.fetch!) and often have a corollary that returns an error tuple.

The difference here, is that the exception is not coming from Repo.transaction. It’s coming from the code you are running inside the thunk; that is your code and your exception. Catching that and turning it into a tuple would be much more surprising to me than raising it.

Maybe the question is really about Repo.insert_all. This function will raise exceptions for all kinds of underlying database conditions including constraint violations, read-only databases, missing tables etc. The conflict behaviour is well-documented in the function docs and is consistent with other Ecto operations. The other exceptions are … well…exceptional. Those are almost always programmer errors and there is no sensible way to handle them other than to crash in my opinion.

Non-bang functions never means it will never raise. For example, pass an integer to File.read/1. They will return tagged tuples for some class of errors that are up to the library to decide.

Plus transaction/2 is running your code via a function, so we shouldn’t compare it with File.read/1. A more apt comparison here would be File.open/2, which also doesn’t handle your own exceptions inside the function either.

That code re-raises everything but the specific :throw on lines 866-868.

Repo.transaction rolls back when an exception occurs but does not stop them. From the docs:

If an unhandled error occurs the transaction will be rolled back and the error will bubble up from the transaction function. If no error occurred the transaction will be committed when the function returns. A transaction can be explicitly rolled back by calling rollback/1, this will immediately leave the function and return the value given to rollback as {:error, value}.

That’s actually fine and allowed. Maybe you misremember something…

The way it works may come as a surprise to you, but it’s not really unsafe, or is it? Any exception that occurs inside a transaction, whether due to the database or Elixir, will cause a rollback of all changes and in that sense it is rather safe. So your idea of safety here seems to be that you expect transaction/2 to always return an error tuple when something went wrong including when an exception was raised.

Why does it work like that? I think it has to do with the concept and the nature of exceptions. The philosophy is namely to let them “bubble” up and not to suppress/convert them at an arbitrary point. Imagine transaction/2 caught all of them and always returned an error tuple. In that case you wouldn’t be able to have an outer/wrapping try/1 statement that listens on certain exceptions. What’s more, such exceptions wouldn’t be reported to the bug database by libraries like Sentry. Unless you pattern-match and re-raise, of course, but you’d have to remember to do that every time you call the transaction function. So in general it’s better to allow exceptions to bubble up.

However, considering how some other functions work you do probably have a point. For example, Jason.decode/2 always returns an error tuple containing the exception struct when there was an error. But there’s the counterpart Jason.decode!/2 which raises in case of errors. Ecto has insert/2 and insert!/2. Should it also have two transaction functions in the same way? Seems reasonable, but it would be a huge breaking change I believe. Maybe you’d like to comment, @josevalim?

If you need to have a “safe” transaction function I’d suggest to add a safe_transaction/2 function to your Repo module which just catches all exceptions and converts them to an error tuple.

Now I wish we’d had a transaction!/2 alternative because letting it raise now means that one would have to spread try/catch’es all over the code for those edge cases, isn’t it? I heard that this could be considered kind of an antipattern exactly because of the premises you exposed.

BTW, the docs say this about the bang convention:

A trailing bang (exclamation mark) signifies a function or macro where failure cases raise an exception.

It doesn’t seem to make a distinction from where or how far down the call stack the exception is raised but actually if the failure case raises an exception or not.

Since transaction/2 abstracts the underlying code, why should I care about what happens inside of it? BTW, you also could make the same argument for the File.read/1 function, because the error doesn’t actually come from inside the function but from erlang / disk - the code must be abstracted away from implementation details somewhere, don’t you think? It seems that it depends on how abstract your code should be.

And now I’m also curious how does Ecto.Multi come to play if this is the expected behavior? I always thought that Ecto.Multi was meant to allow you to do “safe” operations (steps) and treat any error that prevents your operation from succeeding - which is still kinda true. But since there are cases that simply raise, this means that one cannot just match on :error and expect that it would just work, right? I’ve successfully used Ecto.Multi in the past and now I’m thinking about unexpected edge cases left behind.