[Java] Luie initialisering

[Raeghin]

Beste allemaal,

Ik ben bezig met een project waarbij ik een Oracle Database gebruik.
De verbinding hierheen wordt geinitialiseerd in de klasse Database

code:

[...]
    /** The data source. */
    private static OracleDataSource ods = null;  

    /**
     * Initialize.
     * 
     * @param dbUrl the database url
     * @param dbUser the database user
     * @param dbPassword the database password
     */
   @SuppressWarnings("deprecation")
   public static void initialize(String dbUrl, String dbUser, String dbPassword) {
        try {
            if (ods == null) {
                DriverManager.registerDriver(new oracle.jdbc.OracleDriver());
    
                ods = new OracleDataSource();
                ods.setURL(dbUrl);
                ods.setUser(dbUser);
                ods.setPassword(dbPassword);
                ods.setConnectionCachingEnabled(true);
            }
        } catch (SQLException ex) {
            throw new DAOException("Init Database Connections" + ex.getMessage());
        }
    }
[..]

De initialize methode zoals hierboven wordt enkel aangeroepen bij het inloggen in de applicatie of het aanmaken van een proef account (dus bij eerste gebruik van de applicatie waar een database connectie voor nodig is).

Om te voorkomen dat de database verbinding twee keer geinitialiseerd wordt heb ik bij het initialiseren een check gedaan of de datasource (ods) nog null is.

Echter als ik findbug run krijg ik dit terug:

code:

Bug: Incorrect lazy initialization and update of static field api.common.Database.ods
Pattern id: LI_LAZY_INIT_UPDATE_STATIC, type: LI, category: MT_CORRECTNESS


This method contains an unsynchronized lazy initialization of a static field. 
After the field is set, the object stored into that location is further updated or accessed. 
The setting of the field is visible to other threads as soon as it is set. 
If the futher accesses in the method that set the field serve to initialize the object, then you have a very 
serious multithreading bug, unless something else prevents any other thread from accessing the stored 
object until it is fully initialized. 

Even if you feel confident that the method is never called by multiple threads, it might be better to not set 
the static field until the value you are setting it to is fully populated/initialized.

Op het moment dat ik de if(ods == null) weghaal verdwijnt deze bugmelding, maar volgens mij gaat hij dan juist bij elke aanroep van initialize een nieuwe datasource aanmaken met dezelfde gegevens, wat dus overbodig is.
De foutmelding kan ik echter niet zo heel veel wijs uit Mijn vraag is dan ook, is dit echt een bug en moet ik het aanpassen of is dit geen echte bug en kan ik het het beste zo laten staan?

Bedankt alvast voor jullie reacties!

[Gerco]

Je kan het eenvoudig oplossen door de OracleDataSource pas toe te kennen aan de static field *na* de initialisatie is gedaan:

Java:

OracleDataSource tmp = new OracleDataSource();
tmp.setURL(dbUrl);
tmp.setUser(dbUser);
tmp.setPassword(dbPassword);
tmp.setConnectionCachingEnabled(true);
ods = tmp

Dit zorgt ervoor dat het object pas beschikbaar is voor andere threads nadat het volledig geïnitialiseerd is.

Daarnaast kun je deze method beter synchronized maken zodat je het niet dubbel initialiseert wanneer twee threads deze method tegelijkertijd aanroepen. Misschien is dat laatste niet mogelijk in jouw applicatie, maar is het niet onverstandig om het toch te doen. Al was het maar om je bedoeling duidelijker te maken.

[ Voor 27% gewijzigd door Gerco op 27-01-2011 09:46 ]

[Remus]

Het probleem is dat in een multithreaded omgeving het mogelijk is dat twee (of meer!) verschillende threads (semi-)tegelijkertijd de initialize methode aanroepen, en allemaal constateren dat ods nog null is en dus allemaal een datasource aanmaken.

Als je zeker weet dat slechts een thread initialize aanroept (bijvoorbeeld een of andere startup), dan is het geen probleem. Als initialize echter door meerdere threads aangeroepen kan worden, dan is het beter om de method synchronized te maken of een vorm van double-checked locking te gebruiken (maar kijk dan wel naar http://en.wikipedia.org/wiki/Double-checked_locking voor een aantal gotchas).

[Raeghin]

Remus en Gerco, bedankt!

Ik ga de methode synchronized proberen te maken, omdat deze zeker door verschillende threats tegelijkertijd aangeroepen kan worden. Nu zal het nog niet vaak gebeuren maar better safe then sorry!

Jullie uitleg is in ieder geval duidelijker als die van FindBug, want daar kwam ik niet zo veel wijs uit!

[Verwijderd]

Lees anders even http://www.cs.umd.edu/~pu...DoubleCheckedLocking.html . In jou geval gaat het weliswaar om een niet synchronized initialisatie. De vraag die jezelf kan stellen heb ik echt lazy-initialisatie nodig. Je creeert namenlijk geen instantie van je Database om er vervolgens kippen mee gaan te houden. Je creeert altijd een instantie om ook iets met de database te doen

code:

    static {
         DriverManager.registerDriver(new oracle.jdbc.OracleDriver());
    }


    /** The data source. */
    private static final OracleDataSource ods = new OracleDataSource();  

    /**
     * Initialize.
     * 
     * @param dbUrl the database url
     * @param dbUser the database user
     * @param dbPassword the database password
     */
   @SuppressWarnings("deprecation")
   public static void initialize(String dbUrl, String dbUser, String dbPassword) {
        try {
          ods.setURL(dbUrl);
          ods.setUser(dbUser);
          ods.setPassword(dbPassword);
          ods.setConnectionCachingEnabled(true);
        } catch (SQLException ex) {
            throw new DAOException("Init Database Connections" + ex.getMessage());
        }

Aangezien ik de rest van de klasse niet kan zien weet ik niet precies wat de doelstelling is, maar mijn gevoel zegt dat het initialiseren van de datasource in een constructor zou moeten gebeuren en tevens gescheiden zou moeten worden van eigenlijk DAO code. Ik noem dit maar even factory.

code:

public class CoolConnectionFactory {

   private final OracleDataSource ods;  


   public CoolConnectionFactory(String dbUrl, String dbUser, String dbPassword) throws DAOException {
        try {
            DriverManager.registerDriver(new oracle.jdbc.OracleDriver());
            ods = new OracleDataSource();  
        ods.setURL(dbUrl);
        ods.setUser(dbUser);
        ods.setPassword(dbPassword);
        ods.setConnectionCachingEnabled(true);
        } catch (SQLException ex) {
            throw new DAOException("Init Database Connections" + ex.getMessage());
        }
   }

   public Connection createConnection() {
    return ods.getConnection();
   }
}

public class MyDAOClass {

   private final CoolConnectionFactory connectionFactory;

   public MyDAOClass(CoolConnectionFactory connectionFactory) {
      this.connectionFactory = connectionFactory;
   }


   private void closeResources(Connection conn, Statement stat, ResultSet rset) {
      if (rset!=null) try { rset.close(); } catch(Exception ignore) {}
      if (stat!=null) try { stat.close(); } catch(Exception ignore) {}
      if (conn!=null) try { conn.close(); } catch(Exception ignore) {}
   }

   public int countMyStuf {
        Connection conn = null;
        try {
             conn = connectionFactory.createConnection();
             stat = conn.createStatement("SELECT count FROM mytable");
             rset = stat.executeQuery();
             if (rset.next()) {
                  return rset.getInt(0);
             }
        } finally {
          closeResources(conn, stat, rset);
        }
   }

}

Hiermee is het namenlijk ook mogelijk om twee factories te maken die met verschillende user, password en schema werken. Helemaal netjes zou zijn als de klasse MyDAOClass slechts een interface krijgt met alleen de methode createConnection krijgt. Voor deze klasse is het theoretisch namenlijk niet interessant met welke database hij werkt (Oracle, MySQL, ...) ... helaas dat sommige SQL statements niet overal hetzelfde zijn.

PS: bovenstaande code is niet getest en kan dus enekele foutjes bevatten.

[Verwijderd]

owh ... ik was een beetje laat

[Raeghin]

DikkeDouwe,

Bedankt voor je input! Alle DAO's staan in aparte klasses voor elk object wat database communicatie nodig heeft. Ik ga eens uitzoeken hoe synchronized initialisatie werkt.
Als ik een aparte instantie van de database klasse aanroep (wat afaik nodig is als ik hem in een constructor zet) en dus voor elke instantie een verbinding moet maken lijkt me dat een last worden die misschien niet nodig is.

Synchronized initialization is iets waar ik niet bekend mee ben, so correct me if i'm wrong

Hier trouwens de gehele Database klasse:

code:

/*
 *  Database.java  
 * 
 *  Copyright (c) 2009 - 2010 GDS Europe BV. All Rights Reserved.
 *   
 */

package api.common;

import java.sql.*;

import oracle.jdbc.OracleResultSet;
import oracle.jdbc.pool.*;

/**
 * The Class Database.
 * 
 * @version     1.0
 * @author      Raeghin
 */
public class Database {
    
    /** The data source. */
    private static OracleDataSource ods = null;  

    /**
     * Initialize.
     * 
     * @param dbUrl the database url
     * @param dbUser the database user
     * @param dbPassword the database password
     */
   @SuppressWarnings("deprecation")
   public static void initialize(String dbUrl, String dbUser, String dbPassword) {
        try {
            if (ods == null) {
                DriverManager.registerDriver(new oracle.jdbc.OracleDriver());
    
                ods = new OracleDataSource();
                ods.setURL(dbUrl);
                ods.setUser(dbUser);
                ods.setPassword(dbPassword);
                ods.setConnectionCachingEnabled(true);
            }
        } catch (SQLException ex) {
            throw new DAOException("Init Database Connections" + ex.getMessage());
        }
    }

    /**
     * Gets the connection.
     * 
     * @return the connection
     * 
     * @throws SQLException the SQL exception
     */
    public static Connection getConnection() throws SQLException {
        return ods.getConnection();
    }
    
    /**
     * Close the database objects.
     * 
     * @param rs the result set
     * @param stmt the statement
     * @param conn the connection
     * 
     * @throws DAOException the DAO exception
     */
    public static void closeDBObjects(ResultSet rs, CallableStatement stmt, Connection conn) throws DAOException {
        try {
            if (rs != null) {
                rs.close();
            }

            if (stmt != null) {
                stmt.close();
            }

            if (conn != null) {
                conn.close();
            }
        } catch (SQLException ex) {
            throw new DAOException("SQLException: " + ex.getMessage());
        }
    }
    
    /**
     * Close the database objects.
     * 
     * @param rs the result set
     * @param stmt the statement
     * @param conn the connection
     * @param ors the oracle result set
     * 
     * @throws DAOException the DAO exception
     */
    public static void closeDBObjects(ResultSet rs, CallableStatement stmt, Connection conn, OracleResultSet ors) throws DAOException {
        try {
            if (ors != null) {
                ors.close();
            }

            if (stmt != null) {
                stmt.close();
            }

            if (conn != null) {
                conn.close();
            }
            
            if (rs != null) {
                rs.close();
            }
        } catch (SQLException ex) {
            throw new DAOException("SQLException: " + ex.getMessage());
        }
    }
}

Edit:

Ik heb me ingelezen over synchronized initialization e.d.
Ik ben met deze oplossing te komen die lijkt te werken:

code:

[..]
    /**
     * Initialize the database connection
     * 
     * @param dbUrl the database url
     * @param dbUser the database user
     * @param dbPassword the database password
     */
    public static void initialize(String dbUrl, String dbUser, String dbPassword){
        if(ods == null)
            constructInitialization(dbUrl, dbUser, dbPassword);
    }
    
    /**
     * Construct initialization 
     * 
     * @param dbUrl the database url
     * @param dbUser the database user
     * @param dbPassword the database password
     */
    @SuppressWarnings("deprecation")
    synchronized static void constructInitialization(String dbUrl, String dbUser, String dbPassword){
        try{
            DriverManager.registerDriver(new oracle.jdbc.OracleDriver());
            
            OracleDataSource tmp = new OracleDataSource(); 
            tmp.setURL(dbUrl); 
            tmp.setUser(dbUser); 
            tmp.setPassword(dbPassword); 
            tmp.setConnectionCachingEnabled(true); 
            ods = tmp;
        }catch(SQLException ex){
            throw new DAOException("Init Database Connections" + ex.getMessage());
        }
    }
[..]

[ Voor 13% gewijzigd door Raeghin op 27-01-2011 12:04 . Reden: Aanpassing m.b.t. verbeterde code ]

[YopY]

Een paar punten:

Wat gebeurt er als ik het volgende doe?

Java:

Database db = new Database()
Connection conn = db.getConnection();

Antwoord: NullPointerException - na initialisatie van het Database object is deze nog niet in een bruikbare staat omdat het interne DataSourceVeld niet geïnitialiseerd is. Dat kun je oplossen door de initialisatie in de constructor te doen, zoals het hoort.

Ten tweede, de naamgeving is niet correct - het is geen Database namelijk, maar een wrapper om een Oracle datasource te initialiseren en er wat functies op los te laten. Zou daar even over nadenken.

Ik ben met deze oplossing te komen die lijkt te werken:
[...snip]

En wat gebeurt er als je twee threads hebben die beide de 'ods == null' check doen? Dan vinden ze beide dat ods == null en gaan ze beide de synchronized methode aanroepen, waardoor je tweemaal de initialisatie krijgt. De eenvoudigste oplossing is om de ods == null controle een tweede keer in de synchronized methode te doen (vandaar double-checked locking - eerst controleren of het moet, dan locken, en dan nogmaals controleren of het nog steeds moet)

[Raeghin]

YopY schreef op donderdag 27 januari 2011 @ 12:10:
Een paar punten:

Wat gebeurt er als ik het volgende doe?

Java:

Database db = new Database()
Connection conn = db.getConnection();

Antwoord: NullPointerException - na initialisatie van het Database object is deze nog niet in een bruikbare staat omdat het interne DataSourceVeld niet geïnitialiseerd is. Dat kun je oplossen door de initialisatie in de constructor te doen, zoals het hoort.

Ik snap je punt. Maar ik maak nooit een instantie aan van de Database klasse, maar roep deze statisch aan.
Dit gebeurd in de AuthenticationController:

code:

@Override
public void init() throws ServletException {
    super.init();

    Database.initialize(getServletContext().getInitParameter("DB URL"),
            getServletContext().getInitParameter("DB User"),
            getServletContext().getInitParameter("DB Password"));
}

Is het een beter idee om dit aan te passen en het in de constructor te zetten?
Ik dacht het statisch aan te roepen omdat er anders elke keer een nieuwe instantie aangemaakt wordt en dus ook automatisch een nieuwe connectie naar de DB (Elke instantie heeft toch zijn eigen ods variabele), of vergis ik me hierin?

Ten tweede, de naamgeving is niet correct - het is geen Database namelijk, maar een wrapper om een Oracle datasource te initialiseren en er wat functies op los te laten. Zou daar even over nadenken.

Ik ga erover nadenken.

En wat gebeurt er als je twee threads hebben die beide de 'ods == null' check doen? Dan vinden ze beide dat ods == null en gaan ze beide de synchronized methode aanroepen, waardoor je tweemaal de initialisatie krijgt. De eenvoudigste oplossing is om de ods == null controle een tweede keer in de synchronized methode te doen (vandaar double-checked locking - eerst controleren of het moet, dan locken, en dan nogmaals controleren of het nog steeds moet)

Ja dit had ik niet goed gezien, heb het aangepast

In ieder geval bedankt voor je reactie, ik probeer er zoveel mogelijk leering uit te trekken

[ Voor 0% gewijzigd door Raeghin op 27-01-2011 13:40 . Reden: Typo ]

[momania]

YopY schreef op donderdag 27 januari 2011 @ 12:10:

En wat gebeurt er als je twee threads hebben die beide de 'ods == null' check doen? Dan vinden ze beide dat ods == null en gaan ze beide de synchronized methode aanroepen, waardoor je tweemaal de initialisatie krijgt. De eenvoudigste oplossing is om de ods == null controle een tweede keer in de synchronized methode te doen (vandaar double-checked locking - eerst controleren of het moet, dan locken, en dan nogmaals controleren of het nog steeds moet)

Of gewoon geen lazy initialization doen. Dat is hier het niveau optimalisaties zoeken waar het totaal geen nut heeft. Gewoon lekker die connection factory bouwen en iedere keer een verbinding openen. Mocht er later teveel verbindingen nodig zijn kan er altijd nog eens naar connection pooling worden gekeken.

Daarbij: double checked locking werkt helemaal niet gegarandeerd vanwege out-of-order writes. Concurrency is al moeilijk genoeg en de meeste programmeurs moeten hun vingers daar niet aan branden, omdat ze vaak meer onvoorspelbare fouten maken dan goed doen. Advies als double checked locking helpt daar niet bij.

[YopY]

momania schreef op donderdag 27 januari 2011 @ 12:28:
[...]

Of gewoon geen lazy initialization doen.

Beste advies tot nu toe. Je zult waarschijnlijk regelmatig je OracleDataSource nodig hebben, en het is geen PHP script dat een looptijd van 100 milliseconden heeft. Initialiseer je ODS bij applicate initialisatie, dan ben je van deze problemen af (static initialisatie, concurrency issues, etc).

Overigens, als ik het me goed herinner kun je gewoon initialisatie in een static blok doen - dat wordt pas uitgevoerd zodra de class waar het static blok in staat voor de eerste keer aangeroepen wordt. Maar kan ik mis hebben en kan effectief hetzelfde zijn als 'zodra de applicatie start'.

[Remus]

momania schreef op donderdag 27 januari 2011 @ 12:28:
[...]
Daarbij: double checked locking werkt helemaal niet gegarandeerd vanwege out-of-order writes. Concurrency is al moeilijk genoeg en de meeste programmeurs moeten hun vingers daar niet aan branden, omdat ze vaak meer onvoorspelbare fouten maken dan goed doen. Advies als double checked locking helpt daar niet bij.

Die gotcha wordt overigens - iig met betrekking tot Java - behandeld in het Wiki artikel waar ik naar link.