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

Mark and Rémy,

On 7/10/26 1:18 PM, Mark Thomas wrote:
> 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).

+1

Rollback failing is pretty disastrous. Commit failing is not unusual.

> 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);
> }

+1 IMHO

This is how application code generally looks.

-chris