sonar - Debo detectar las excepciones lanzadas al cerrar java.sql.Connection

sonar code review

Question

Connection.close() puede lanzar SqlException , pero siempre he supuesto que es seguro ignorar tales excepciones (y nunca he visto código que no las ignore).

Normalmente escribiría:

 try{
    connection.close();
 }catch(Exception e) {}

O

 try{
    connection.close();
 }catch(Exception e) {
     logger.log(e.getMessage(), e); 
 }

La pregunta es:

1. ¿Es una mala práctica (y alguien ha tenido problemas al ignorar tales excepciones).
2. Cuando Connection.close() lanza cualquier excepción.
3. Si es malo, ¿cómo debo manejar la excepción?

Comentario:

Sé que descartar excepciones es malo, pero solo estoy refiriéndome a las excepciones lanzadas al cerrar una conexión (y como he visto, esto es bastante común en este caso).

¿Alguien sabe cuándo Connection.close() puede arrojar algo?

Answer 1

En un mundo ideal, nunca debes hacer nada en una excepción, por supuesto, en un mundo ideal, nunca obtendrás una excepción, ya sea 8-)

Entonces, debes examinar los impactos de las diversas opciones.

Solo registro: las operaciones de la base de datos están todas terminadas, no queda nada por hacer, excepto limpiar los recursos. Si ocurre una excepción en este punto, lo más probable es que no tenga impacto en el trabajo realizado, por lo que registrar el error debería ser suficiente. Por supuesto, si se produce un error durante el registro, básicamente debe manejar la operación fallida de la base de datos que no falló realmente.

Controlador vacío: las operaciones de la base de datos han terminado, no queda nada más que limpiar los recursos. Si se produce una excepción en este punto, lo más probable es que no tenga ningún impacto en el trabajo realizado, por lo que el método regresa con éxito. El siguiente acceso a la base de datos puede tener el mismo problema, pero debería ocurrir al comienzo de una transacción, donde con razón no se logrará y luego se manejará de manera adecuada. Si el problema se solucionó, no habrá indicios de que algo haya salido mal.

Es un escenario bastante típico para poner una operación close () en un bloque finally para asegurar que se realice la limpieza, ya que no queremos ningún otro error para inhibir la limpieza de recursos. Si no se ha producido ningún error, entonces su método no debería fallar cuando su operación se haya completado con éxito. En este caso, el manejo de excepciones vacías es bastante normal.

Por supuesto, las opiniones variarán.

Answer 2

Personalmente, me gusta su segunda idea de, al menos, registrar el error. Debido a que está capturando Excepción, teóricamente es posible detectar algo que no sea una Excepción de SQL. No estoy seguro de qué podría pasar o qué tan raro (como excepciones de falta de memoria, etc.), pero no me parece correcto suprimir todos los errores.

Si desea suprimir los errores, lo haría solo a los muy específicos que usted sabe que deberían ser tratados de esa manera.

Situación hipotética: ¿qué pasaría si su SQL tuviera una transacción abierta y el cierre de la conexión ocasionara un error debido a eso? ¿Desea suprimir ese error? Incluso la supresión de SQLExceptions podría ser un poco peligrosa.

Answer 3

Por mi experiencia, ignorar una excepción nunca es una buena idea. Créame, los ingenieros de soporte de producción y los analistas le agradecerán muchísimo si registra la excepción.

Además, si está utilizando el marco de trabajo de registro correcto, la excepción tendrá un impacto de rendimiento nulo o mínimo.

Answer 4

Tienes que manejar la excepción. No es una mala práctica. Imagine que perdió la red justo antes de cerrar la conexión dabatase. Probablemente lanzará la excepción.

¿Es raro? Sí. Supongo que eso es lo que se llaman excepciones y esa no es una razón para ignorarlo. Recuerde que si pudiera fallar, fallará.

También debería pensar si es posible tener una conexión nula en este punto (podría causar una NullPointerException) o no.

if (connection != null) {
   try { 
      connection.close(); 
   } catch (SQLException sqle) { 
      logger.log(e.getMessage(), e); 
   }
}

Answer 5

Como mínimo, siempre siempre siempre se registran las excepciones que está atrapando y no actuando.

Las excepciones capturadas silenciosamente que se tragan sin el más mínimo pío son las peores.

Answer 6

En realidad, lo que estás haciendo (casi) es la mejor práctica :-) esto es lo que vi en JdbcUtils.java de Spring. Por lo tanto, es posible que desee agregar otro bloque de captura.

/**
 * Close the given  ResultSet and ignore any thrown exception.
 * This is useful for typical finally blocks in manual  code.
 * @param resultSet the  ResultSet to close
 * @see javax.resource.cci.ResultSet#close()
 */
private void closeResultSet(ResultSet resultSet) {
  if (resultSet != null) {
    try {
      resultSet.close();
    }
    catch (SQLException ex) {
      logger.debug("Could not close  ResultSet", ex);
    }
    catch (Throwable ex) {
      // We don''t trust the  driver: It might throw RuntimeException or Error.
      logger.debug("Unexpected exception on closing  ResultSet", ex);
    }
  }
}

Answer 7

También podría lanzar una RuntimeException:

try {
    connection.close();
 } catch(Exception e) {
     throw new RuntimeException(e); 
 }

No tendrá que cambiar la firma de su método y podrá usar el método Exception.getCause más adelante para encontrar la causa del problema.

Answer 8

si este es un caso de "error que nunca ocurre", entonces volveré a lanzar una Excepción y espero que nadie la atrape.
si este es otro caso, probablemente lo registre

Answer 9

En general, he tenido días desperdiciados por personas que arrojan excepciones como esa.

Recomiendo seguir algunas reglas básicas con excepciones:

Si está ABSOLUTAMENTE SEGURO de que NUNCA causará un problema con una excepción marcada, capture SÓLO esa excepción y comente exactamente por qué no necesita manejarla. (Sleep arroja una InterruptedException que siempre se puede ignorar a menos que realmente le interese, pero sinceramente este es el único caso que suelo ignorar, incluso en ese caso, si nunca lo obtiene, ¿cuál es el costo de iniciar sesión?)

Si no está seguro, pero puede obtenerlo de vez en cuando, capture y registre un seguimiento de la pila solo de modo que si está causando un problema, se puede encontrar. Nuevamente, capture solo la excepción que necesita.

Si no ve la forma de lanzar la excepción marcada, tómela y vuelva a lanzarla como una excepción no verificada.

Si sabe exactamente qué está causando la excepción, tóquela y registre exactamente por qué, realmente no necesita un seguimiento de la pila en este caso si tiene muy claro qué es lo que la causa (y puede mencionar la clase que está iniciando sesión si no estás usando log4j o algo así.

Parece que su problema caería en la última categoría, y para este tipo de captura, nunca haga lo que escribió (Excepción e), siempre haga la excepción específica en caso de que se genere alguna excepción no verificada (parámetros incorrectos, puntero nulo, ...)

Actualización: el principal problema aquí es que las excepciones controladas no son buenas. El único lenguaje altamente usado en el que existen es Java. En teoría son limpios, pero en la práctica causan este comportamiento de captura y ocultación que no se obtiene con excepciones no verificadas.

Mucha gente ha comentado sobre el hecho de que dije que esconderlos a veces está bien. Para ser específico, el único caso en el que puedo pensar es:

try {
    Thread.sleep(1000);
catch (InterruptedException e) {
    // I really don''t care if this sleep is interrupted!
}

Supongo que la razón principal por la que siento que este uso está bien es porque este uso de InterruptedException es un abuso del patrón de excepción comprobado, en primer lugar, está comunicando el resultado de un sueño más que indicando una condición de excepción.

Tendría mucho más sentido tener:

boolean interrupted=Thread.sleep(1000);

Pero estaban muy orgullosos de su nuevo patrón de excepción comprobada cuando crearon Java por primera vez (comprensiblemente, es realmente ingenioso, solo falla en la práctica)

No puedo imaginar otro caso en el que esto sea aceptable, así que tal vez debería haberlo enumerado como el único caso en el que podría ser válido ignorar una excepción.

Answer 10

Tenga en cuenta que Apache Commons DButils proporciona un método closeQuietly() , que puede usar para evitar saturar su código con capturas "redundantes". Tenga en cuenta que no estoy abogando por tragar excepciones, pero para este escenario close() creo que es generalmente aceptable.

Answer 11

Si puede manejarlo, hágalo (y conéctelo si fue inesperado). Si no puede manejarlo, vuelva a lanzarlo correctamente para que algún código anterior pueda manejarlo.

Discretamente tragar excepciones está dejando de lado la información crucial para que la persona corrija el código.

Answer 12

Es una mejor práctica manejar la excepción al momento de cerrar la conexión a la base de datos. Debido a que, más adelante, en algún punto del código, si está intentando acceder a la declaración o a los objetos del conjunto de resultados, generará automáticamente una excepción. Entonces, es mejor manejar la excepción.