Skip to content

Conversation

zhfeng
Copy link
Contributor

@zhfeng zhfeng commented Mar 28, 2018

The race condition is in the ManagedConnection when closing the
connection. This could case the connection leak during the transaction
in high load.

@markt-asf
Copy link
Contributor

Looks reasonable to me.
Rather than a sync on this, please use a dedicated private Object to lock on.

The race condition is in the ManagedConnection when closing the
connection. This could case the connection leak during the transaction
in high load.
@zhfeng
Copy link
Contributor Author

zhfeng commented Mar 30, 2018

Thank @markt-asf for reviewing and I've changed to use the private lock. Also add some comments to make clearer.

@PascalSchumacher
Copy link
Contributor

@markt-asf Is this ready to merge?

@markt-asf
Copy link
Contributor

Looks good to me

@zhfeng
Copy link
Contributor Author

zhfeng commented May 9, 2018

@garydgregory can it be include in the new release ?

@garydgregory
Copy link
Member

Hi @zhfeng,

The release VOTE for 2.3.0 based on RC1 just got underway. If the VOTE passes, this PR will have to be considered for the next release after that.

Also, to be honest, I had forgotten about this PR :-(

Note that the fact this PR does not contain a unit test makes it less likely for consideration.

Ideally, we would like a unit test that fails without the main patch. Then after applying the main patch, the unit test passes.

@zhfeng
Copy link
Contributor Author

zhfeng commented May 9, 2018

Thanks @garydgregory , I'm working on the unit test by using the byteman tool which is LGPL. So it is OK to use it just in the tests ?

@garydgregory
Copy link
Member

Why not write a real test using an H2 database? http://www.h2database.com/html/main.html

@garydgregory
Copy link
Member

WRT byteman, I am not 100% sure if it is OK to link to it from test code. That will need some research from someone on the Apache side.

@markt-asf
Copy link
Contributor

Thanks for the updated PR. I was thinking sync on new Object() but a Lock is fine too. Looks good to me.

@garydgregory
Copy link
Member

LGPL 2.1 is listed here https://www.apache.org/legal/resolved.html under "WHICH LICENSES MAY NOT BE INCLUDED WITHIN APACHE PRODUCTS?"

@zhfeng zhfeng closed this May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants