Bug 63833 - NPE in DBCP when attempting to connect to a database that doesn't exist
Summary: NPE in DBCP when attempting to connect to a database that doesn't exist
Status: NEW
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Integration (show other bugs)
Version: 7.0.96
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-10-10 23:32 UTC by Adam Rauch
Modified: 2019-10-12 13:20 UTC (History)
0 users



Attachments
A draft to solve this issue. (614 bytes, patch)
2019-10-11 13:12 UTC, Guoxiong Li
Details | Diff
A new draft patch for review. (2.47 KB, patch)
2019-10-12 07:03 UTC, Guoxiong Li
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Rauch 2019-10-10 23:32:29 UTC
When attempting to connect to a DataSource that specifies a database that doesn't exist, Tomcat 7.0.96 throws a NullPointerException with no useful information. See below for stack trace.

Calling code is simply:

    try (Connection conn = dataSource.getConnection())
    {
    ...
    } 

In this same situation, Tomcat 8.5.46 throws an informative SQLException; for example: java.sql.SQLException: Cannot create PoolableConnectionFactory (FATAL: database "labkey19.3" does not exist)

An exception is appropriate, but the 8.5.46/DBCP2 behavior is obviously preferable, since it gives administrators a better understanding of what's gone wrong.

Reported to the user mailing list and was encouraged to file this bug.

Thanks,
Adam

Tomcat 7.0.96

java.lang.NullPointerException
    at org.apache.tomcat.dbcp.dbcp.PoolableConnectionFactory.destroyObject(PoolableConnectionFactory.java:643)
    at org.apache.tomcat.dbcp.dbcp.BasicDataSource.validateConnectionFactory(BasicDataSource.java:1738)
    at org.apache.tomcat.dbcp.dbcp.BasicDataSource.createPoolableConnectionFactory(BasicDataSource.java:1721)
    at org.apache.tomcat.dbcp.dbcp.BasicDataSource.createDataSource(BasicDataSource.java:1486)
    at org.apache.tomcat.dbcp.dbcp.BasicDataSource.getConnection(BasicDataSource.java:1103)
    at org.labkey.api.data.DbScope.<init>(DbScope.java:337)
    at org.labkey.api.data.DbScope.addScope(DbScope.java:1275)
    at org.labkey.api.data.DbScope.initializeScopes(DbScope.java:1243)
    at org.labkey.api.module.ModuleLoader.initializeDataSources(ModuleLoader.java:1004)
    at org.labkey.api.module.ModuleLoader.doInit(ModuleLoader.java:352)
    at org.labkey.api.module.ModuleLoader.init(ModuleLoader.java:249)
    at org.apache.catalina.core.ApplicationFilterConfig.initFilter(ApplicationFilterConfig.java:285)
    at org.apache.catalina.core.ApplicationFilterConfig.getFilter(ApplicationFilterConfig.java:266)
    at org.apache.catalina.core.ApplicationFilterConfig.<init>(ApplicationFilterConfig.java:108)
    at org.apache.catalina.core.StandardContext.filterStart(StandardContext.java:5037)
    at org.apache.catalina.core.StandardContext.startInternal(StandardContext.java:5739)
    at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:145)
    at org.apache.catalina.core.ContainerBase.addChildInternal(ContainerBase.java:1018)
    at org.apache.catalina.core.ContainerBase.addChild(ContainerBase.java:994)
    at org.apache.catalina.core.StandardHost.addChild(StandardHost.java:662)
    at org.apache.catalina.startup.HostConfig.deployDescriptor(HostConfig.java:712)
    at org.apache.catalina.startup.HostConfig$DeployDescriptor.run(HostConfig.java:2002)
    at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
    at java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:264)
    at java.base/java.util.concurrent.FutureTask.run(FutureTask.java)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:830)

Tomcat 8.5.46

java.sql.SQLException: Cannot create PoolableConnectionFactory (FATAL: database "labkey19.3" does not exist)
    at org.apache.tomcat.dbcp.dbcp2.BasicDataSource.createPoolableConnectionFactory(BasicDataSource.java:669)
    at org.apache.tomcat.dbcp.dbcp2.BasicDataSource.createDataSource(BasicDataSource.java:544)
    at org.apache.tomcat.dbcp.dbcp2.BasicDataSource.getConnection(BasicDataSource.java:753)
    at org.labkey.api.data.DbScope.<init>(DbScope.java:337)
    at org.labkey.api.data.DbScope.addScope(DbScope.java:1275)
    at org.labkey.api.data.DbScope.initializeScopes(DbScope.java:1243)
    at org.labkey.api.module.ModuleLoader.initializeDataSources(ModuleLoader.java:1004)
    at org.labkey.api.module.ModuleLoader.doInit(ModuleLoader.java:352)
    at org.labkey.api.module.ModuleLoader.init(ModuleLoader.java:249)
    at org.apache.catalina.core.ApplicationFilterConfig.initFilter(ApplicationFilterConfig.java:283)
    at org.apache.catalina.core.ApplicationFilterConfig.getFilter(ApplicationFilterConfig.java:264)
    at org.apache.catalina.core.ApplicationFilterConfig.<init>(ApplicationFilterConfig.java:108)
    at org.apache.catalina.core.StandardContext.filterStart(StandardContext.java:4546)
    at org.apache.catalina.core.StandardContext.startInternal(StandardContext.java:5191)
    at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:150)
    at org.apache.catalina.core.ContainerBase.addChildInternal(ContainerBase.java:743)
    at org.apache.catalina.core.ContainerBase.addChild(ContainerBase.java:719)
    at org.apache.catalina.core.StandardHost.addChild(StandardHost.java:714)
    at org.apache.catalina.startup.HostConfig.deployDescriptor(HostConfig.java:614)
    at org.apache.catalina.startup.HostConfig$DeployDescriptor.run(HostConfig.java:1823)
    at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
    at java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:264)
    at java.base/java.util.concurrent.FutureTask.run(FutureTask.java)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:830)
Caused by: org.postgresql.util.PSQLException: FATAL: database "labkey19.3" does not exist
    at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2440)
    at org.postgresql.core.v3.QueryExecutorImpl.readStartupMessages(QueryExecutorImpl.java:2559)
    at org.postgresql.core.v3.QueryExecutorImpl.<init>(QueryExecutorImpl.java:133)
    at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:250)
    at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:49)
    at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:195)
    at org.postgresql.Driver.makeConnection(Driver.java:454)
    at org.postgresql.Driver.connect(Driver.java:256)
    at org.apache.tomcat.dbcp.dbcp2.DriverConnectionFactory.createConnection(DriverConnectionFactory.java:55)
    at org.apache.tomcat.dbcp.dbcp2.PoolableConnectionFactory.makeObject(PoolableConnectionFactory.java:355)
    at org.apache.tomcat.dbcp.dbcp2.BasicDataSource.validateConnectionFactory(BasicDataSource.java:115)
    at org.apache.tomcat.dbcp.dbcp2.BasicDataSource.createPoolableConnectionFactory(BasicDataSource.java:665)
    ... 25 more
Comment 1 Guoxiong Li 2019-10-11 13:12:01 UTC
Created attachment 36823 [details]
A draft to solve this issue.
Comment 2 Phil Steitz 2019-10-11 22:31:52 UTC
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.
Comment 3 Guoxiong Li 2019-10-12 07:03:04 UTC
Created attachment 36824 [details]
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.
Comment 4 Christopher Schultz 2019-10-12 12:19:57 UTC
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?
Comment 5 Guoxiong Li 2019-10-12 13:17:43 UTC
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.
Comment 6 Konstantin Kolinko 2019-10-12 13:20:06 UTC
(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.