Fix leaking connections when it was not possible to close them.

[rPraml]

DB2 has a problem, that it won't allow to close connections if there is an uncommitted UOW.

This PR adresses the following issues:

1. When an active connection is returned to the pool. e.g. by this code block

      try (Connection connection = pool.getConnection()) {
        // we do nothing here.
      }

a warning is logged, that commit/rollback was missing. We also try to roll back this connection, as in the worst case, data can leak to the next usage.

This is done here: https://github.com/ebean-orm/ebean-datasource/compare/master...FOCONIS:ebean-datasource:db2-tests?expand=1#diff-916f1a5da1ef6f478d7cb23223fc9bfd2fb0c64ff6b0484f399c19403c6ddb8fR457

2. When shutting down the pool, we always try to roll back the connection first, before it will be closed. If this is not done, DB2 will not close the transaction, if there is still UOW open. This can happen, as various commands like getSchema can start a new UOW an already committed/rollbacked connection. The connection will be never closed and the tcp/ip connection kept open until the JVM will restart
   See test testFalseFriendRollback

3. We changed the invocation of DataSourcePoolListener.onBeforeReturnConnection to the beginning of the whole reset process of autocommit/isolationLevel/catalog/schema/status=IDLE.
   This allows us to install a custom DB2-listener. While 1 + 2 will cover 95% of leaking connections, the listener can check isInDB2UnitOfWork and handle the other 5%.

This might be considered as behaviour change, if you think, onBeforeReturnConnection should be the last method, I can add a onBeforeResetConnection to the listener.

I know, the behaviour of DB2-JCC driver is a bit odd here (and could theoretically be configured with connectionCloseWithInFlightTransaction parameter) - but it is as it is. And we had major outages last week.

@rbygrave It would be great, if we get feedback about these changes

/edit: What happened exactly:

This change 94f9f71#diff-916f1a5da1ef6f478d7cb23223fc9bfd2fb0c64ff6b0484f399c19403c6ddb8fR125 caused the trouble.

• If the system was under the correct load, so that for the heartbeat thread a newly created connection was issued, the getSchema caused, that this connection had an "open UOW".
• Now the "testConnection" just verifies, if this connection is OK, performs neither a commit, nor a rollback and put this connection back on the pool
• Now we have an "uncloseable" connection in the pool. If this connection gets trimmed/closed, an error is detected and the connection was removed from the pool, but keeps the tcp/ip connection open until server restart.

The change to getSchema() in the initialization (which is already removed in 9.2) makes this scenario more likely.

So now, we have either improved the close logic (to try a rollback) and also do a rollback in the heartbeat

https://github.com/ebean-orm/ebean-datasource/pull/107/files#diff-17a97b637b84f5d3e16f487dcd4a8d80d7cdb9f165b528cdc4d495cc93070647R380

Note: The JDBC-documentation says https://docs.oracle.com/javase/8/docs/api/java/sql/Connection.html#close--

It is strongly recommended that an application explicitly commits or rolls back an active transaction prior to calling the close method. If the close method is called and there is an active transaction, the results are implementation-defined.

So I think, we can enforce this behaviour. I also think we may throw an exception instead of the Log.trace

[rPraml]

please review #108, #109 and #110 first. I then will rebase this PR

[rPraml]

Hello @rbygrave,
I've rebased this PR and thanks for merging the other PRs.

FYI: we think that the getSchema() call (introduced by me in 9.1 and removed in 9.2) was partly responsible for causing an outage in one of our production systems under the right amount of load in combination with the DB2 driver ;)

[rPraml]

CHECKME: nothing was the old behaviour.

Now we do a rollback (which is recommeded by the JDBC-API) if the application code did not already rollback/commit the txn.
So in some cases, an additional command is sent to the db (=maybe slower)

Note: Ebean does the job already very well, and in 95% only committed/rollbacked connections are closed.

For example ImplicitReadOnlyTransaction does a commit at the end. See: ebean-orm/ebean@3e24190

These are the 5% left, where a transaction is put back to pool without commit/rollback. See: ebean-orm/ebean#3579

[rbygrave]

My thought here is that rollback and fail are really the only options we should allow.

[rPraml]

Ok, let's think again about the use cases

1. nothing this would be the old behaviour. This is error prone and can result in uncommitted data not being committed until the next use. So, let’s discard the possibility of switching back to the old procedure
2. rollback this option is absolutely necessary and should become the default option
3. commit this option is actually nonsense and I can't imagine a useful use case
4. remove also seems to be nonsense and that there are dangling connections in the heap
5. fail This is for testing and it is highly recommended to enable it in unit tests. As mentioned in the comment, this should not be used in production as try-with-resources will stop working under unexpected conditions (e.g. when an exception is thrown).
   We found a few places in our code where we forgot to commit the transaction and are wondering how this code could work (probably the commit was executed the next time the connection was used).

the more I think about it, I agree and think it is better to convert closeWithinTxn to a boolean enforceCleanClose or assertCleanClose, so that we have the functionality of rollback and fail.

I also would log this as a warning when we do an unexpected rollback.

[rbygrave]

so that we have the functionality of rollback and fail.

Yeah, I think this sounds like the right approach at this stage.

[rPraml]

unfortunately. properties does not support enums, yet. Maybe this should be changed?

[rbygrave]

Happy with this. Thinking we could simplify this to only allow ROLLBACK and FAIL options.

[rbygrave]

My thought here is that rollback and fail are really the only options we should allow.

[rPraml]

Hello @rbygrave I've refactored this PR to support only fail/rollback.

Documentation is provided in #116
I'm unsure about the following things:

• ensureCleanClose or assertCleanClose or do you have a better name?
• what exception should be thrown, when enabled?
  • SqlException? May be catched and swallowed
  • IllegalStateException? May be also catched
  • AssertionError? My preference (then I also should rename the property)
• Should I log the situation as warning without stack trace?

Especially the last may flood the log, but hey, these were really dangerous programming errors... (In our code, exactly that happend, that we could not explain, why data was committed without commit statement)

And as far as I can say, if you use ebean-datasource with ebean (+ this PR ebean-orm/ebean#3579) you should be safe.