On 10/07/2026 16:45, Rémy Maucherat wrote:
On Fri, Jul 10, 2026 at 5:13 PM Mark Thomas <[email protected]> wrote:
On 10/07/2026 13:17, [email protected] wrote:
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 8120a12a0d Fixes from code review
8120a12a0d is described below
commit 8120a12a0dbbaafc104fa0d61a319d480b7ff92c
Author: remm <[email protected]>
AuthorDate: Fri Jul 10 14:17:44 2026 +0200
Fixes from code review
Take advantage of rollback to improve save reliability.
Clear out transient memory changes only if save is successful,
Line 1253: There is no rollback if the commit fails
I don't see the point. What should I do then if rollback fails. And so
on. LLMs often go on a "what if it fails" loop and there's no good
solution.
Commit failing strikes me as more likely than rollback failing
(essentially rollback shouldn't fail).
I'd expected something like:
dbConnection.setAutoCommit(false);
try {
saveInternal();
commit();
] catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
try {
rollback();
} catch (SQLException sqle) {
t.addSuppressed(sqle);
throw t
}
} finally {
// clear out transient here
dbConnection.setAutoCommit(true);
}
With all the catch (SQLException e) and returning exception removed from
saveIntenal() since I don't think any of those exceptions are going to
be thrown until commit() anyway.
Also, if commit fails, setAutoCommit(true) is going to trigger another
attempt at committing the changes. The pattern above avoids that because
there is always a rollback after a failure (assuming rollback never
fails which seems reasonable).
Line 1731: Some of these fields were optional (and are still marked as
such in the documentation). That migth be a breaking change for some odd
configurations. No objections to the change if a) the breaking change is
documented in the migration guide and b) the docs are updated
Ok, I see userRoleTable and roleNameCol should be semi optional (so
users without roles). I will make some adjustments.
Thanks.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
