C'è un problema con la chiusura delle connessioni del nostro database nel blocco "Finalmente" di un'istruzione Try?

7

Sto eseguendo alcuni refactoring per la nostra applicazione e cercando di ridurre il numero di problemi segnalati nella nostra scansione ISO (uno strumento di analisi del codice statico basato su HP Fortify). In questo momento, quello che sto cercando di risolvere è il problema "Unreleased Resource: Database" che la nostra applicazione ha creato. Uno dei motivi più importanti per questo è costrutti come questo:

Connection conn = null;
Boolean myConn = false;
try{
   if(conn == null){
     conn = DatabaseUtil.getConnection();
     myConn = true;
   }
   result = DbClass.getObject(conn, otherParameters);
}catch(DatabaseException de){
  throw de;
}catch(SQLException sqle){
  throw new DatabaseException("Error Message");
}finally{
  if(myConn && conn != null){
    try{
      conn.rollback();
      conn.close();
    }catch(SQLException sqle){}
}

Questo è un costrutto abbastanza standard nella nostra applicazione per garantire che la connessione al database sia chiusa alla fine del nostro blocco Try. Eppure ogni volta che lo usiamo, lo strumento lo segnala come un problema.

C'è qualcosa di sbagliato in questo costrutto, e c'è qualcosa che possiamo fare per risolverlo se si tratta di un problema?

    
posta Zibbobz 04.06.2015 - 16:56
fonte

3 risposte

10

E gli strumenti sono giusti!

Oltre ai (possibili) limiti dell'analisi - può o non può capire che (conn != null) == myConn , osserva che rollback genera un SqlException . Se lo fa, allora close non viene mai chiamato , che è precisamente la definizione di una perdita di risorse. Ora, potrebbe essere il caso che con il tuo corrente DB, i driver e le librerie di pool se rollback getta, allora la connessione è già stata rilasciata, ma questo non è vero nel caso generale, quindi un potenziale problema.

Come notato nei commenti, è possibile semplificare gran parte di questo costrutto utilizzando try-with-resources se si utilizza Java 7. Diventa anche molto più semplice ragionare e includere un rollback tramite una classe personalizzata che esegue il rollback nel suo metodo close() in un blocco try:

@Override
public void close() throws SQLException {
    try {
        rollback();
    catch (...) {
        //Do stuff
    finally {
        super.close();
    }
}

Qui sei garantito al 100% che close() verrà effettivamente eseguito, a prescindere da rollback .

Se lavori con software legacy, il codice sopra riportato può essere inserito direttamente in try :

Connection conn = null;
try {
   conn = DatabaseUtil.getConnection();
   result = DbClass.getObject(conn, otherParameters);
} catch(SQLException sqle) {
  throw new DatabaseException("Error Message");
} finally {
  if (conn != null) {
    try{
        conn.rollback();
    } catch(SQLException sqle) {
        //try-with-resources would add this to suppressed exceptions, you can either log or rethrow
    } finally {
        conn.close();
    }
}
    
risposta data 04.06.2015 - 17:50
fonte
5

Poiché il tuo rilascio di risorse è condizionato dallo stato di una variabile booleana e racchiuso in un altro blocco try , l'analizzatore statico deve decidere che rollback() e close() non sono garantiti per l'esecuzione.

Controlla la documentazione per l'oggetto Connection del tipo restituito dal metodo getConnection() factory, e verifica se i metodi rollback() e close() genereranno persino un'eccezione. Se non lo faranno, non avrai bisogno di quel blocco aggiuntivo try catch quando chiami quei metodi.

Ma prima, elimina la variabile booleana che stai usando per segnalare una connessione aperta. Non dovresti averne bisogno; non si dovrebbe recuperare un oggetto di connessione dal metodo getConnection() factory a meno che non abbia avuto successo nello stabilire una connessione.

Si noti che il report sulla sicurezza di Fortify suggerisce la seguente struttura di codice:

public void execCxnSql(Connection conn) {
    Statement stmt;
    try {
        stmt = conn.createStatement();
        ResultSet rs = stmt.executeQuery(CXN_SQL);
        ...
    } 
    finally {
        if (stmt != null) {
            safeClose(stmt);
        }
    }
}
public static void safeClose(Statement stmt) {
    if (stmt != null) {
        try {
            stmt.close();
        } catch (SQLException e) {
            log(e);
        }
    }
}

... che suggerisce che la variabile booleana è il problema, non il blocco aggiuntivo try .

    
risposta data 04.06.2015 - 17:30
fonte
0

Elimina la condizione, dal momento che il compilatore non può eseguire il codice.

Quindi cattura Exception invece di SQLException in modo che recuperi NullPointerException nel caso in cui le connessioni non siano inizializzate. Di solito l'oggetto viene annullato dopo averlo chiuso, perché ho ricevuto una raccomandazione in un documento EAServer di Sybase e, forse, non ha nulla a che fare con il garbage collector di Java ma con il pool di connessioni di EAServer e forse sono "cargo -culting " qui.

Connection conn = null;
Boolean myConn = false;
try{
   if(conn == null){
     conn = DatabaseUtil.getConnection();
     // myConn = true; no need for this
   }
   result = DbClass.getObject(conn, otherParameters);
}catch(DatabaseException de){
  throw de;
}catch(SQLException sqle){
  throw new DatabaseException("Error Message");
}finally{
    try{
      conn.rollback();
      conn.close();
      conn=null; // good practice according to people at Sybase
    }catch(Exception e){
         //fails silently when conn is not initialized.
    }
}
    
risposta data 04.06.2015 - 18:01
fonte

Leggi altre domande sui tag