First, read AG-227

 

Whilst providing a big improvement, the connection management changes still appear to contain a subtle flaw that will leak connections under certain circumstances.

 

https://github.com/agroal/agroal/blob/2.3/agroal-narayana/src/main/java/io/agroal/narayana/RecoveryXAResource.java#L58

 

if ( flag == TMENDRSCAN && ( value == null || value.length == 0 ) )

{                 close(); }

 

This is designed around two assumptions: that it's invalid to close a connection during a recovery scan, since that invalidates the cursor it's using in the db, and that the connection won't be used again if the results of the scan contained no elements. Those are fine, but...

It also assumes that if the result set is non-empty, the connection will be used again. Unfortunately that's untrue in two respects. First, it doesn't actually fix the MS SQL issue that AG-227 was aimed at directly, but since other methods support reconnection now anyhow that's ok: In the case that TMSTARTRSCAN returns results but TMENDRSCAN does not, the connection may still be required, but will be reopened if so.

Second and more important, there are cases where the result set is non-empty and yet the connection will not be used again. These still leak. The candidate fix is

 

if ( flag == TMENDRSCAN ) {                 close(); }

 

which is a bit eager on the closing, but that's better than not eager enough, since the cost of reconnecting is not high.