Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DBCP-515 Added support for obtaining a reference to the connection du… #16

Closed
wants to merge 1 commit into from

Conversation

tomjenkinson
Copy link

@tomjenkinson tomjenkinson commented Jul 20, 2018

https://issues.apache.org/jira/browse/DBCP-515

Should be merged with #17 ideally to ensure that the connection is not double returned to the pool during checkOpen and similar (existing bug)

@kinow
Copy link
Member

kinow commented Jul 21, 2018

Same comment as from #15. I don't know enough about this change to merge it, but also, the following tests are not passing locally:

[INFO] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0 s - in org.apache.commons.dbcp2.TestPStmtKey
[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   TestManagedDataSourceInTx.testGetConnectionInAfterCompletion:85 Could not get connection
[INFO] 
[ERROR] Tests run: 1403, Failures: 1, Errors: 0, Skipped: 4
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

@tomjenkinson tomjenkinson force-pushed the DBCP-515 branch 2 times, most recently from 186518a to 0c522aa Compare July 23, 2018 09:49
@tomjenkinson
Copy link
Author

I have pushed a fix to correct the test. What the patch is about is basically to not try to register the synchronization when the transaction on the thread is already committed, as would be the case in afterCompletion for example. During testing I see that some frameworks can try to do a getConnection().getWarnings() in afterCompletion. If they attempt that at the moment they will get an exception saying that it is not allowed to register the synchronization and so that can be protected against in this library.

@garydgregory
Copy link
Member

@tomjenkinson,

May you please rebase this patch on top of git master now that I've applied PR #15?

Thank you!

@tomjenkinson
Copy link
Author

@garydgregory - I have rebased it now and retested locally thanks

@garydgregory
Copy link
Member

garydgregory commented Jul 25, 2018

@tomjenkinson,

Thank you for rebasing your patch.

Unfortunately it breaks binary compatibility, which you can check by running mvn clirr:check:

[INFO] --- clirr-maven-plugin:2.8:check (default-cli) @ commons-dbcp2 ---
[INFO] artifact org.apache.commons:commons-dbcp2: checking for updates from apache.snapshots
[INFO] artifact org.apache.commons:commons-dbcp2: checking for updates from central
[INFO] Comparing to version: 2.5.0
[ERROR] 7004: org.apache.commons.dbcp2.managed.ManagedConnection$CompletionListener: In method 'public void afterCompletion(org.apache.commons.dbcp2.managed.TransactionContext, boolean)' the number of arguments has changed
[ERROR] 7004: org.apache.commons.dbcp2.managed.TransactionContextListener: In method 'public void afterCompletion(org.apache.commons.dbcp2.managed.TransactionContext, boolean)' the number of arguments has changed

Can you provide a patch that keeps the old API in place? With an appropriate behavior?

Thank you!
Gary

@tomjenkinson
Copy link
Author

Sure, sorry about that. I saw that the argument was not being used in the only implementation in the code base and that code appeared to be internal so I thought to prune at the same time.

That said, there is a reasonable approach to use and still retain the parameter so I have pushed an update.

Thanks for looking

@asfgit asfgit closed this in bc7a396 Jul 25, 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
3 participants