Comment by AdieuToLogic
6 days ago
> Having the retry logic within the `finally` block seems like the best way of denoting the intention to me, since the intention is to swallow the result, whether that was a return or a throw.
Except that is not the documented intent of the `finally` construct:
The finally block always executes when the try block exits.
This ensures that the finally block is executed even if an
unexpected exception occurs. But finally is useful for more
than just exception handling — it allows the programmer to
avoid having cleanup code accidentally bypassed by a
return, continue, or break. Putting cleanup code in a
finally block is always a good practice, even when no
exceptions are anticipated.[0]
Using `finally` for implementing retry logic can be done, as you have illustrated, but that does not mean it is "the best way of denoting the intention." One could argue this is a construct specific to Java (the language) and does not make sense outside of this particular language-specific idiom.
Conceptually, "retries" are not "cleanup code."
0 - https://docs.oracle.com/javase/tutorial/essential/exceptions...
Sounds like the right intent to me. To pinpoint your existing quote from the documentation:
> The finally block always executes when the try block exits. This ensures that the finally block is executed even if an unexpected exception occurs.
The intent of the transaction code is that the consistency is checked (using `tx.commit()`) "even if an unexpected exception occurs".
I'm not sure how else to interpret that to be honest. If you've got a clearer way of expressing this, feel free to explain.
> The intent of the transaction code is that the consistency is checked (using `tx.commit()`) "even if an unexpected exception occurs".
A transaction failing is the opposite of an unexpected event. Transactions failing is a central use case of any transaction. Therefore it should be handled explicitly instead of using exceptions.
Exceptions are for unexpected events such as the node running out of memory, or a process failing to write to disk.
> A transaction failing is the opposite of an unexpected event.
That's why it's denoted by a non-exceptional return value from `tx.commit()` in my sample code. When I've talked about exceptions here, I'm talking about exceptions raised within the transaction. If the transaction succeeds, those exceptions should be propagated to the calling code.
> Exceptions are for unexpected events such as the node running out of memory, or a process failing to write to disk.
Discussing valid uses of exceptions seems orthogonal to this (should OOM lead to a catchable exception [0], or should it crash the process?). In any case, if the process is still alive and the transaction code determines without error that "yes, this transaction was invalid due to other contending transactions", it should retry the transaction. If something threw due to lack of memory or disk space, chances are it will throw again within a successful transaction and the error will be propagated.
[0] As alluded to in my first post, you might want to add some special cases for exceptions/errors that you want to immediately propagate instead of retrying. Eg, you might treat `Error` subtypes differently, which includes `OutOfMemoryError` and other cases that suggest the program is in a potentially unusable state, but this still isn't required according to the intent of the transactional logic.
> The intent of the transaction code is that the consistency is checked (using `tx.commit()`) "even if an unexpected exception occurs".
First, having a `commit` unconditionally attempted when an exception is raised would surprise many developers. Exceptions in transactional logic are often used to represent a "rollback persistent store changes made thus far" scenario.
Second, using a condition within `finally` to indicate a retry due to a `commit` failing could be expressed in a clearer manner by having it within the `try` block as described by IntelliJ here[0].
0 - https://www.jetbrains.com/help/inspectopedia/ContinueOrBreak...
> Exceptions in transactional logic are often used to represent a "rollback persistent store changes made thus far" scenario.
Handling can be added to change the transaction to be read-only if the inner code throws a particular exception, but the consistency should still be checked through a `commit` phase (at least in an OCC setting), so the `continue` in `finally` is still the correct way to do it.
> could be expressed in a clearer manner by having it within the `try` block as described by IntelliJ here[0].
> 0 - https://www.jetbrains.com/help/inspectopedia/ContinueOrBreak...
Wrong link? The only solution I see there is to add a comment to suppress the warning, which sounds fine to me (eg, analogous to having a `// fallthrough` comment when intentionally omitting `break` statements within `switch`, since I can agree that both of these things are uncommon, but sometimes desirable).
1 reply →