Re: (tomcat) branch main updated: Fixes from code review

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