[Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833 Mark Thomas changed: What|Removed |Added Resolution|--- |FIXED Status|NEW |RESOLVED --- Comment #7 from Mark Thomas --- Thanks for the patch. I applied the fix but not the unit test as Phil Steitz has back-ported the Commons DBCP 1.x unit tests that also catch this error. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
On 10/16/19 7:55 AM, Mark Thomas wrote:
On 12/10/2019 00:03, Phil Steitz wrote:
How about adding the DBCP unit tests to the source tree? I suspect some
would have failed due to this change. If others think this is a good
idea, I could take a stab at genericising them and creating a PR to add
them.
+1
I'd suggest several commits. Something like:
- Copy latest 1.x from Commons
- Fix package naming issues
- Fix any running issues
- Fix warnings (inc. generics)
That way it should be easy to trace what changed from Commons and why.
The exact detail of how the changes are split between commits is not
important. It is the easy traceability that matters.
I just submitted a PR with the BasicDataSource tests and its
dependencies with commits in the order above. I disabled tests that
were failing due to this issue (BZ 63833). If this looks OK, I will
continue to add the rest of them. I can also do a separate PR to fix BZ
63833 and re-enable the failing tests. I could not find a 1.6 JDK to
test with, so my testing was with 1.7 and 1.8 using test.name to limit
execution to the tests that I was adding.
Phil
Mark
On 10/11/19 3:31 PM, bugzi...@apache.org wrote:
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833
Phil Steitz changed:
What |Removed |Added
OS| |All
--- Comment #2 from Phil Steitz ---
This is a regression from the generics conversion in
PoolableConnectionFactory.
The original DBCP 1.x code effectively null-checked the object to be
destroyed:
public void destroyObject(Object obj) throws Exception {
if(obj instanceof PoolableConnection) {
((PoolableConnection)obj).reallyClose();
}
Removing the instanceOf check makes NPE possible:
public void destroyObject(PoolableConnection obj) throws Exception {
obj.reallyClose();
}
Solution is to add an explicit null check in destroyObject.
Similar changes should be made to activate, passivate, validate methods.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
On 10/16/19 7:55 AM, Mark Thomas wrote:
On 12/10/2019 00:03, Phil Steitz wrote:
How about adding the DBCP unit tests to the source tree? I suspect some
would have failed due to this change. If others think this is a good
idea, I could take a stab at genericising them and creating a PR to add
them.
+1
I'd suggest several commits. Something like:
- Copy latest 1.x from Commons
- Fix package naming issues
- Fix any running issues
- Fix warnings (inc. generics)
That way it should be easy to trace what changed from Commons and why.
The exact detail of how the changes are split between commits is not
important. It is the easy traceability that matters.
Got it. I have started working on this. I will likely submit PRs that
bundle commits in the sequence above for one or more test classes at a
time.
Phil
Mark
On 10/11/19 3:31 PM, bugzi...@apache.org wrote:
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833
Phil Steitz changed:
What |Removed |Added
OS| |All
--- Comment #2 from Phil Steitz ---
This is a regression from the generics conversion in
PoolableConnectionFactory.
The original DBCP 1.x code effectively null-checked the object to be
destroyed:
public void destroyObject(Object obj) throws Exception {
if(obj instanceof PoolableConnection) {
((PoolableConnection)obj).reallyClose();
}
Removing the instanceOf check makes NPE possible:
public void destroyObject(PoolableConnection obj) throws Exception {
obj.reallyClose();
}
Solution is to add an explicit null check in destroyObject.
Similar changes should be made to activate, passivate, validate methods.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
On 12/10/2019 00:03, Phil Steitz wrote:
> How about adding the DBCP unit tests to the source tree? I suspect some
> would have failed due to this change. If others think this is a good
> idea, I could take a stab at genericising them and creating a PR to add
> them.
+1
I'd suggest several commits. Something like:
- Copy latest 1.x from Commons
- Fix package naming issues
- Fix any running issues
- Fix warnings (inc. generics)
That way it should be easy to trace what changed from Commons and why.
The exact detail of how the changes are split between commits is not
important. It is the easy traceability that matters.
Mark
>
> On 10/11/19 3:31 PM, bugzi...@apache.org wrote:
>> https://bz.apache.org/bugzilla/show_bug.cgi?id=63833
>>
>> Phil Steitz changed:
>>
>> What |Removed |Added
>>
>>
>> OS| |All
>>
>> --- Comment #2 from Phil Steitz ---
>> This is a regression from the generics conversion in
>> PoolableConnectionFactory.
>>
>> The original DBCP 1.x code effectively null-checked the object to be
>> destroyed:
>> public void destroyObject(Object obj) throws Exception {
>> if(obj instanceof PoolableConnection) {
>> ((PoolableConnection)obj).reallyClose();
>> }
>>
>> Removing the instanceOf check makes NPE possible:
>> public void destroyObject(PoolableConnection obj) throws Exception {
>> obj.reallyClose();
>> }
>>
>> Solution is to add an explicit null check in destroyObject.
>>
>> Similar changes should be made to activate, passivate, validate methods.
>>
>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833 --- Comment #6 from Konstantin Kolinko --- (In reply to Christopher Schultz from comment #4) It is much better to keep discussion in one place in Bugzilla, rather than scattered among maybe several PRs. I was wondering why this bug report is against Tomcat instead of the upstream Apache Commons DBCP project, by Phil Steitz's comment 2 gives an explanation. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833 --- Comment #5 from Guoxiong Li --- I once submitted a PR to solve a issue. But the PR was not good enough to merge so that the committer solved the issue by himself and closed my PR without merging. As a result, I attach the patch to this BZ for committers to review and comment this time so that I can modify my patch and avoid my PR to be rejected. It is better for committer to comment, give some recommendation and let the PR submitter to modify the PR instead of doing similar thing by themselves after reviewing the PR. Maybe the situation of my previous PR happened by accident and is not the typical review process. I will choose only one way(patch or PR) to contribute in the future according to your suggestion which could avoid repeat. In this BZ, I will just use the patch file and won't use PR. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833 --- Comment #4 from Christopher Schultz --- Both reviews and edits can all be done in a PR. Why attach successive patches to this BZ issue when you plan in issuing a PR anyway? -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833 Guoxiong Li changed: What|Removed |Added Attachment #36823|0 |1 is obsolete|| --- Comment #3 from Guoxiong Li --- Created attachment 36824 --> https://bz.apache.org/bugzilla/attachment.cgi?id=36824=edit A new draft patch for review. Hi Phil Steitz, think your for your reminding. I update my patch. The new patch add null check in method destroyObject. I don't modify method activateObject, validateConnection and passivateObject because I consider that a patch should minumize and avoid too many changes. When the reviewer agree to the change and the test method in this path, I will add another patch which changes the method activateObject, validateConnection and passivateObject. I will squash all the commit and submit a pull request which make it easy to merge. Please review the patch. Thanks. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
How about adding the DBCP unit tests to the source tree? I suspect some
would have failed due to this change. If others think this is a good
idea, I could take a stab at genericising them and creating a PR to add
them.
On 10/11/19 3:31 PM, bugzi...@apache.org wrote:
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833
Phil Steitz changed:
What|Removed |Added
OS||All
--- Comment #2 from Phil Steitz ---
This is a regression from the generics conversion in PoolableConnectionFactory.
The original DBCP 1.x code effectively null-checked the object to be destroyed:
public void destroyObject(Object obj) throws Exception {
if(obj instanceof PoolableConnection) {
((PoolableConnection)obj).reallyClose();
}
Removing the instanceOf check makes NPE possible:
public void destroyObject(PoolableConnection obj) throws Exception {
obj.reallyClose();
}
Solution is to add an explicit null check in destroyObject.
Similar changes should be made to activate, passivate, validate methods.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833
Phil Steitz changed:
What|Removed |Added
OS||All
--- Comment #2 from Phil Steitz ---
This is a regression from the generics conversion in PoolableConnectionFactory.
The original DBCP 1.x code effectively null-checked the object to be destroyed:
public void destroyObject(Object obj) throws Exception {
if(obj instanceof PoolableConnection) {
((PoolableConnection)obj).reallyClose();
}
Removing the instanceOf check makes NPE possible:
public void destroyObject(PoolableConnection obj) throws Exception {
obj.reallyClose();
}
Solution is to add an explicit null check in destroyObject.
Similar changes should be made to activate, passivate, validate methods.
--
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 63833] NPE in DBCP when attempting to connect to a database that doesn't exist
https://bz.apache.org/bugzilla/show_bug.cgi?id=63833 --- Comment #1 from Guoxiong Li --- Created attachment 36823 --> https://bz.apache.org/bugzilla/attachment.cgi?id=36823=edit A draft to solve this issue. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
