Commons Dbcp
  1. Commons Dbcp
  2. DBCP-309

First example for FSContext is invalid

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 1.2.2
    • Fix Version/s: 1.5.1, 2.0
    • Labels:
      None

      Description

      First example on page http://commons.apache.org/dbcp/guide/jndi-howto.html is invalid, with this code every call of

      InitialContext ic2 = new InitialContext();
      DataSource ds = (DataSource) ic2.lookup("jdbc/basic");
      Connection conn = ds.getConnection();
      conn.close();

      ends with new datasource with unclosed connection in it becase new Reference("javax.sql.DataSource", "org.apache.commons.dbcp.BasicDataSourceFactory", null); reference creates new DataSource instead using created one while calling ic2.lookup("jdbc/basic").

      At the end it ends with many opened connections to DB until JVM is ended.

      Second example I didn't test, i use direct aproach with manualy creating SharedPoolDataSource and registring it in FSContext JNDI itself.

        Activity

        Hide
        Phil Steitz added a comment -

        Looks like a bug in the examples to me. Looks like the BasicDataSource example is taken from an example (Tomcat?) where a more general type of resource is being created but not bound directly into the JNDI context. Unless I am missing something, it would be simpler (and correct) to just create the datasource instance and bind it directly.

        Show
        Phil Steitz added a comment - Looks like a bug in the examples to me. Looks like the BasicDataSource example is taken from an example (Tomcat?) where a more general type of resource is being created but not bound directly into the JNDI context. Unless I am missing something, it would be simpler (and correct) to just create the datasource instance and bind it directly.
        Hide
        Phil Steitz added a comment -

        Strike last comment. DataSoources do not implement Referenceable, so direct references will not work. When using the FileSystem provider, datasources are created new for each lookup (as reported in the issue). Tomcat's in-memory provider, however, returns the same datasource for repeated lookups. The following test, for example, succeeds with the Tomcat provider, but fails with the FS provider

        Hashtable environment = new Hashtable();
                environment.put(Context.INITIAL_CONTEXT_FACTORY,
                        org.apache.naming.java.javaURLContextFactory.class.getName());
                InitialContext ic = new InitialContext(environment);
                ic.createSubcontext("jdbc"); 
                // Construct BasicDataSource reference
                Reference ref = new Reference("javax.sql.DataSource",
                  "org.apache.commons.dbcp.BasicDataSourceFactory", null);
                ref.add(new StringRefAddr("driverClassName", "TesterDriver"));
                ref.add(new StringRefAddr("url", "jdbc:apache:commons:testdriver"));
                ref.add(new StringRefAddr("username", "username"));
                ref.add(new StringRefAddr("password", "password"));
                ic.rebind("jdbc/basic", ref);
                 
                // Use
                InitialContext ic2 = new InitialContext(environment);
                DataSource ds = (DataSource) ic2.lookup("jdbc/basic");
                ((BasicDataSource) ds).setAccessToUnderlyingConnectionAllowed(true);
                Assert.assertNotNull(ds);
                Connection conn = ds.getConnection();
                Connection uconn = ((DelegatingConnection) conn).getInnermostDelegate();
                Assert.assertNotNull(conn);
                conn.close();
                
                DataSource ds2 = (DataSource) ic2.lookup("jdbc/basic");
                ((BasicDataSource) ds2).setAccessToUnderlyingConnectionAllowed(true);
                Assert.assertNotNull(ds2);
                Connection conn2 = ds2.getConnection();
                Connection uconn2 = ((DelegatingConnection) conn2).getInnermostDelegate();
                Assert.assertNotNull(conn2);
                conn2.close();
                Assert.assertEquals(ds, ds2);
                Assert.assertEquals(uconn2, uconn);
        

        To make the examples less misleading, it seems we have two choices - 1) change to use the Tomcat provider or 2) comment that the FS provider creates new datasources each time. Would appreciate comments / patches from JNDI spi experts on this.

        Show
        Phil Steitz added a comment - Strike last comment. DataSoources do not implement Referenceable, so direct references will not work. When using the FileSystem provider, datasources are created new for each lookup (as reported in the issue). Tomcat's in-memory provider, however, returns the same datasource for repeated lookups. The following test, for example, succeeds with the Tomcat provider, but fails with the FS provider Hashtable environment = new Hashtable(); environment.put(Context.INITIAL_CONTEXT_FACTORY, org.apache.naming.java.javaURLContextFactory.class.getName()); InitialContext ic = new InitialContext(environment); ic.createSubcontext( "jdbc" ); // Construct BasicDataSource reference Reference ref = new Reference( "javax.sql.DataSource" , "org.apache.commons.dbcp.BasicDataSourceFactory" , null ); ref.add( new StringRefAddr( "driverClassName" , "TesterDriver" )); ref.add( new StringRefAddr( "url" , "jdbc:apache:commons:testdriver" )); ref.add( new StringRefAddr( "username" , "username" )); ref.add( new StringRefAddr( "password" , "password" )); ic.rebind( "jdbc/basic" , ref); // Use InitialContext ic2 = new InitialContext(environment); DataSource ds = (DataSource) ic2.lookup( "jdbc/basic" ); ((BasicDataSource) ds).setAccessToUnderlyingConnectionAllowed( true ); Assert.assertNotNull(ds); Connection conn = ds.getConnection(); Connection uconn = ((DelegatingConnection) conn).getInnermostDelegate(); Assert.assertNotNull(conn); conn.close(); DataSource ds2 = (DataSource) ic2.lookup( "jdbc/basic" ); ((BasicDataSource) ds2).setAccessToUnderlyingConnectionAllowed( true ); Assert.assertNotNull(ds2); Connection conn2 = ds2.getConnection(); Connection uconn2 = ((DelegatingConnection) conn2).getInnermostDelegate(); Assert.assertNotNull(conn2); conn2.close(); Assert.assertEquals(ds, ds2); Assert.assertEquals(uconn2, uconn); To make the examples less misleading, it seems we have two choices - 1) change to use the Tomcat provider or 2) comment that the FS provider creates new datasources each time. Would appreciate comments / patches from JNDI spi experts on this.
        Hide
        Ondrej Tisler added a comment -

        I use this code for registring JNDI inFSContext:
        // JNDI with datasource
        InitialContext ic = new InitialContext();
        // Construct BasicDataSource reference
        Configuration poolConfig = dbConfig.subset("datasource");
        DriverAdapterCPDS cpds = new DriverAdapterCPDS();
        cpds.setDriver(poolConfig.getString("driver.driverClassName"));
        cpds.setUrl(poolConfig.getString("driver.url"));
        cpds.setUser(poolConfig.getString("driver.username"));
        cpds.setPassword(CryptPass.encryptPass(poolConfig.getString("driver.password")));
        SharedPoolDataSource ds = new SharedPoolDataSource();
        ds.setConnectionPoolDataSource(cpds);
        ds.setDefaultAutoCommit(poolConfig.getBoolean("connection.defaultAutoCommit", true));
        ds.setValidationQuery(poolConfig.getString("validation.validationQuery"));
        ic.rebind(poolConfig.getString("[@jndi]"), ds);
        log.info("JNDI datasource is inicialized");

        next you can normaly use JNDI lookup:
        InitialContext ic2 = new InitialContext();
        ds = (DataSource) ic2.lookup(poolConfig.getString("[@jndi]"));
        conn = ds.getConnection();

        Show
        Ondrej Tisler added a comment - I use this code for registring JNDI inFSContext: // JNDI with datasource InitialContext ic = new InitialContext(); // Construct BasicDataSource reference Configuration poolConfig = dbConfig.subset("datasource"); DriverAdapterCPDS cpds = new DriverAdapterCPDS(); cpds.setDriver(poolConfig.getString("driver.driverClassName")); cpds.setUrl(poolConfig.getString("driver.url")); cpds.setUser(poolConfig.getString("driver.username")); cpds.setPassword(CryptPass.encryptPass(poolConfig.getString("driver.password"))); SharedPoolDataSource ds = new SharedPoolDataSource(); ds.setConnectionPoolDataSource(cpds); ds.setDefaultAutoCommit(poolConfig.getBoolean("connection.defaultAutoCommit", true)); ds.setValidationQuery(poolConfig.getString("validation.validationQuery")); ic.rebind(poolConfig.getString(" [@jndi] "), ds); log.info("JNDI datasource is inicialized"); next you can normaly use JNDI lookup: InitialContext ic2 = new InitialContext(); ds = (DataSource) ic2.lookup(poolConfig.getString(" [@jndi] ")); conn = ds.getConnection();
        Hide
        Phil Steitz added a comment -

        Unfortunately, I think the right fix here is to drop the BasicDataSource example and resolve this issue, but open another one to make BasicDataSource implement Referenceable in 2.0.

        Show
        Phil Steitz added a comment - Unfortunately, I think the right fix here is to drop the BasicDataSource example and resolve this issue, but open another one to make BasicDataSource implement Referenceable in 2.0.
        Hide
        Mark Thomas added a comment -

        Caveat: I am no JNDI expert. The little I do know is gleaned from reverse engineering Tomcat's JNDI implementation.

        I think the example is wrong because it binds a reference. A reference will always result in a new object being created when it is looked up. If the example binds a BasicDataSource instance, it appears to behave as desired/expected.

        As most users expect multiple JNDI look ups using the same name to return the same object, Tomcat deliberately breaks the rules. By default, when Tomcat finds a reference, Tomcat resolves that reference to an object and then replaces the reference in JNDI with the object. In later Tomcat versions, this is configurable with the singleton property.

        It isn't clear to me how implementing Referenceable will help. If anything, that will make it worse as it would then be impossible to bind an instance of a BasicDataSource to JNDI as the Reference would be bound.

        I intend to tweak the examples to bind instances rather than References. This appears to be consistent with Ondrej Tisler's approach above.

        Show
        Mark Thomas added a comment - Caveat: I am no JNDI expert. The little I do know is gleaned from reverse engineering Tomcat's JNDI implementation. I think the example is wrong because it binds a reference. A reference will always result in a new object being created when it is looked up. If the example binds a BasicDataSource instance, it appears to behave as desired/expected. As most users expect multiple JNDI look ups using the same name to return the same object, Tomcat deliberately breaks the rules. By default, when Tomcat finds a reference, Tomcat resolves that reference to an object and then replaces the reference in JNDI with the object. In later Tomcat versions, this is configurable with the singleton property. It isn't clear to me how implementing Referenceable will help. If anything, that will make it worse as it would then be impossible to bind an instance of a BasicDataSource to JNDI as the Reference would be bound. I intend to tweak the examples to bind instances rather than References. This appears to be consistent with Ondrej Tisler's approach above.
        Hide
        Mark Thomas added a comment -

        I've fixed trunk for the 2.x series the 1.5.x branch. It will be included in the next release of each.

        If there are further 1.3.x and 1.4.x releases (currently being discussed on the dev mailing list) my expectation is that they will be generated from the 1.5.x branch so will pick up this fix too.

        Show
        Mark Thomas added a comment - I've fixed trunk for the 2.x series the 1.5.x branch. It will be included in the next release of each. If there are further 1.3.x and 1.4.x releases (currently being discussed on the dev mailing list) my expectation is that they will be generated from the 1.5.x branch so will pick up this fix too.

          People

          • Assignee:
            Unassigned
            Reporter:
            Ondrej Tisler
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development