Fix: memory leak in transaction handles

[lucio-santi]

Transaction handles are currently allocated upon starting new transactions without proper deallocations of previous handles, which leads to memory leaks. This PR fixes this issue following an approach seen in other OCI driver implementations: a single transaction handle is reused for every new transaction.

Note: somehow, one test case (TestSelectDualString) seems to have a non-deterministic behavior, as it might occasionally fail even in the master branch. All other tests passed normally. We also tested this fix under realistic loads in one of our apps and everything ran smoothly.

[MichaelS11]

Thank you for working on this and looking into this. Have a couple of questions.

1) How do you know transactions are leaking memory? Have you run some kind of test? If so, can you reproduce that test?

2) Where is it noted that transaction handles need to be freed?

Also there is not a lot of testing around transactions. Will most likely need to add more testing with transaction. One test in particular is that three or more transactions at the same time will work correctly. Probably a few other new tests would be good as well.

[lucio-santi]

Hi Michael!

Thank you for your prompt reply. I've just created a new issue (#411) to contextualize this. Apologies for doing it after creating this PR. Regarding your questions,

1. As stated in the issue, we found significant memory leaks in one of our apps and we strongly think they are due to this. New requests are permanently creating and starting new transactions, and after inspecting the code we found their handles are not freed. We ran stress tests before and after the fix and the leak is definitely gone.
2. It is actually not noted anywhere, but not doing it leads to the aforementioned issues. We have inspected some other implementations that make use of OCI drivers and they seem to reuse a single handle as proposed in this PR. Unfortunately, official docs (e.g. here) do not provide greater detail, but at least they seem to be consistent with this fix.

Please let us know if you need further details or information!

Best regards

[MichaelS11]

If a single transaction handle can be used, can it be created always so nill check is not needed?

I think the only test in the code for transaction is TestDestructiveTransaction. Maybe more testing is needed?

[lucio-santi]

It should be possible to allocate new transaction handles instead of reusing a single transaction context, but instead of checking against nil we should instead deallocate the handle somewhere --possibly after committing or rollbacking the transaction. I think this approach adds overhead and complexity for current use cases.

As for the tests, I can surely add a couple more cases so that this fix is properly covered.

[MichaelS11]

Not sure if you are following what I am saying. Was saying move conn.ociHandleAlloc(C.OCI_HTYPE_TRANS, 0) out of BeginTx and into database open.

Wonderful :)

[lucio-santi]

That's right, I misunderstood your sentence. Thanks for the feedback!

[lucio-santi]

Sure, tests are not done yet --this was in fact a first commit, still WIP. Thanks for your input, I'll let you know once everything is done on our side.

[lucio-santi]

Michael, I've been reviewing the test case you mentioned earlier today (TestDestructiveTransaction) and it covers several features, even operating with two transactions. I've just pushed an extension to this test with two extra transactions running after the first ones and one of them being eventually rollbacked. Since we do not introduce new functionality here, I believe this test should be enough to cover this PR.

Have a nice weekend and thanks for your time!