Derby
  1. Derby
  2. DERBY-633

synchronization issues in client code

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 10.1.1.0
    • Fix Version/s: None
    • Component/s: Network Client
    • Urgency:
      Normal

      Description

      I ran a static race detection tool to check the implementation of the following java.sql.* interfaces in Derby and found 319 possible bugs (static instances of missing synchronization):

      Blob
      Clob
      DatabaseMetaData
      Connection
      Statement
      PreparedStatement
      CallableStatement
      ResultSet

      I ran the tool in 2 passes. The results of the 2 passes are here:

      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS1/
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS2/

      PASS1 reports "shallow" races, which I fixed and re-ran the tool, and PASS2 reports any remaining deep races. I fixed those as well and re-ran the tool, and it did not report any more races. The 2 passes are because a single instance of bad synchronization can cause multiple races (and, conversely, a single race can indicate multiple instances of bad synchronization), so to prevent overwhelming the user with race reports, the first pass checks only for races on fields of top-level objects (ex. NetConnection) but the second pass does a full-blown check.

      ====================================================================
      Analysis of results of PASS 1:
      ====================================================================

      ===============================
      java.sql.Blob

      0 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS1/blob/index.html

      ===============================
      java.sql.Clob

      0 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS1/clob/index.html

      ===============================
      java.sql.Connection

      147 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS1/connection/index.html

      These are due to lack of synchronization on 'this' in 12 methods in class Connection:

      boolean getAutoCommit()
      boolean isClosed()
      int getTransactionIsolation()
      java.sql.SQLWarning getWarnings()
      java.sql.DatabaseMetaData getMetaData()
      boolean isReadOnly()
      String getCatalog()
      java.util.Map getTypeMap()
      int getHoldability()
      java.sql.PreparedStatement prepareStatement(String sql, int autoGeneratedKeys)
      java.sql.PreparedStatement prepareStatement(String sql, int columnIndexes[])
      java.sql.PreparedStatement prepareStatement(String sql, String columnNames[])

      In most of the above cases, it is the classic situation of possibly incorrect synchronization in which the programmer leaves get* methods unsychronized. It may or may not be correct depending upon what those get* methods are doing. I leave it to the programmer to decide in this case. Also, it is not sufficient to reason at the Java level: given the subtleties of the Java memory model, one needs to reason at the bytecode level (see the next item for more on this).

      ===============================
      java.sql.DatabaseMetaData

      6 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS1/database_metadata/index.html

      All are on field metaDataInfoIsCached_ of class DatabaseMetaData due to lack of synchronization on connection_ in 107 public methods in this class that call a getMetaData* method. One of these 107 methods is:

      public boolean supportsBatchUpdates() throws SqlException

      { checkForClosedConnection(); return getMetaDataInfoBoolean(supportsBatchUpdates__); }

      The programmer seems to be aware of these races b/w the protected write access to field metaDataInfoIsCached_ in the below method

      // We synchronize at this level so that we don't have to synchronize all
      // the meta data info methods. If we just return hardwired answers we
      // don't need to synchronize at the higher level.
      private void metaDataInfoCall() {
      synchronized (connection_)

      { ... // update metaDataInfoCache_ metaDataInfoIsCached_ = true; }

      }

      and the unprotected read access to that field in each of the getMetaData* methods, for instance:

      private String getMetaDataInfoString(int infoCallIndex) {
      if (metaDataInfoIsCached_)

      { return (String) metaDataInfoCache_[infoCallIndex]; }


      ...
      }

      The races are benign if one reasons at the Java level, but they might be worth looking at from the perspective of the bytecode level. In particular, since a JVM is free to reorder instructions within a synchronized block, it is legal (though highly unlikely) for it to move the write to metaDataInfoIsCached_ in the metaDataInfoCall method to before the "..." code that updates the elements of array metaDataInfoCache_, in which case there is clearly a bug. I myself am not very familiar with the Java memory model, but see http://www.cs.umd.edu/users/pugh/java/memoryModel/jsr-133-faq.html for some unexpected things that can happen due to reordering of bytecode which is legal within (but not across) a synchronization block.

      ===============================
      java.sql.Statement

      20 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS1/statement/index.html

      These are due to lack of synchronization on connection_ in 14 methods in class Statement:

      int getMaxFieldSize()
      int getMaxRows()
      int getQueryTimeout()
      void cancel()
      java.sql.SQLWarning getWarnings()
      int getFetchDirection()
      int getFetchSize()
      int getResultSetConcurrency()
      int getResultSetType()
      java.sql.Connection getConnection()
      java.sql.ResultSet getGeneratedKeys()
      int executeUpdate(String sql, int columnIndexes[])
      boolean execute(String sql, int columnIndexes[])
      int getResultSetHoldability()

      Most of these are due to unsynchronized get* methods (similar to Connection above).

      ===============================
      java.sql.PreparedStatement

      0 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS1/prepared_statement/index.html

      ===============================
      java.sql.CallableStatement

      26 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS1/callable_statement/index.html

      All are on field wasNull_ due to lack of synchronization on connection_ in the wasNull method in class CallableStatement.

      ===============================
      java.sql.ResultSet

      764 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS1/result_set/index.html

      These are due to lack of synchronization on connection_ in 87 methods in class ResultSet:

      boolean wasNull()
      boolean getBoolean(int column)
      byte getByte(int column)
      short getShort(int column)
      int getInt(int column)
      long getLong(int column)
      float getFloat(int column)
      double getDouble(int column)
      java.math.BigDecimal getBigDecimal(int column, int scale)
      java.math.BigDecimal getBigDecimal(int column)
      java.sql.Date getDate(int column)
      java.sql.Date getDate(int column, java.util.Calendar calendar)
      java.sql.Time getTime(int column)
      java.sql.Time getTime(int column, java.util.Calendar calendar)
      java.sql.Timestamp getTimestamp(int column)
      java.sql.Timestamp getTimestamp(int column, java.util.Calendar calendar)
      String getString(int column)
      byte[] getBytes(int column)
      java.io.InputStream getBinaryStream(int column)
      java.io.InputStream getAsciiStream(int column)
      java.io.InputStream getUnicodeStream(int column)
      java.io.Reader getCharacterStream(int column)
      java.sql.Blob getBlob(int column)
      java.sql.Clob getClob(int column)
      java.sql.Ref getRef(int column)
      java.sql.Array getArray(int column)
      Object getObject(int column)
      Object getObject(int column, java.util.Map map)
      final boolean getBoolean(String columnName)
      final byte getByte(String columnName)
      final short getShort(String columnName)
      final int getInt(String columnName)
      final long getLong(String columnName)
      final float getFloat(String columnName)
      final double getDouble(String columnName)
      final java.math.BigDecimal getBigDecimal(String columnName, int scale)
      final java.math.BigDecimal getBigDecimal(String columnName)
      final java.sql.Date getDate(String columnName)
      final java.sql.Date getDate(String columnName, java.util.Calendar cal)
      final java.sql.Time getTime(String columnName)
      final java.sql.Time getTime(String columnName, java.util.Calendar cal)
      final java.sql.Timestamp getTimestamp(String columnName)
      final java.sql.Timestamp getTimestamp(String columnName, java.util.Calendar cal)
      final String getString(String columnName)
      final byte[] getBytes(String columnName)
      final java.io.InputStream getBinaryStream(String columnName)
      final java.io.InputStream getAsciiStream(String columnName)
      final java.io.InputStream getUnicodeStream(String columnName)
      final java.io.Reader getCharacterStream(String columnName)
      final java.sql.Blob getBlob(String columnName)
      final java.sql.Clob getClob(String columnName)
      final java.sql.Array getArray(String columnName)
      final java.sql.Ref getRef(String columnName)
      final Object getObject(String columnName)
      final Object getObject(String columnName, java.util.Map map)
      final java.sql.SQLWarning getWarnings()
      java.sql.ResultSetMetaData getMetaData()
      boolean isBeforeFirst()
      boolean isAfterLast()
      boolean isFirst()
      boolean isLast()
      int getFetchDirection()
      int getFetchSize()
      int getType()
      int getConcurrency()
      boolean rowUpdated()
      boolean rowInserted()
      boolean rowDeleted()
      void updateNull(String columnName)
      void updateBoolean(String columnName, boolean x)
      void updateByte(String columnName, byte x)
      void updateShort(String columnName, short x)
      void updateInt(String columnName, int x)
      void updateLong(String columnName, long x)
      void updateFloat(String columnName, float x)
      void updateDouble(String columnName, double x)
      void updateBigDecimal(String columnName, java.math.BigDecimal x)
      void updateDate(String columnName, java.sql.Date x)
      void updateTime(String columnName, java.sql.Time x)
      void updateTimestamp(String columnName, java.sql.Timestamp x)
      void updateString(String columnName, String x)
      void updateBytes(String columnName, byte x[])
      void updateBinaryStream(String columnName, java.io.InputStream x, int length)
      void updateAsciiStream(String columnName, java.io.InputStream x, int length)
      void updateCharacterStream(String columnName, java.io.Reader x, int length)
      void updateObject(String columnName, Object x, int scale)
      void updateObject(String columnName, Object x)

      Most of these races seem to be intentional, for instance, many (but not all) of them contain the following comments:

      // Live life on the edge and run unsynchronized
      public boolean getBoolean(int column)

      { ... setWasNull(column); // Placed close to the return to minimize risk of thread interference return result; }

      But as I said above, it might be worth looking at these races because of unexpected transformations that the JVM might do at the bytecode level. Clearly, the second comment in the above method suggests that the programmer assumes absence of aggressive transformations.

      ====================================================================
      Analysis of results of PASS 2:
      ====================================================================

      ===============================
      java.sql.Blob

      0 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS2/blob/index.html

      ===============================
      java.sql.Clob

      0 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS2/clob/index.html

      ===============================
      java.sql.Connection

      51 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS2/connection/index.html

      These races are on fields so deep inside some class (java.nio.Buffer) that I cannot verify them. But synchronizing method nativeSQL in class Connection on 'this' eliminates them.

      ===============================
      java.sql.ResultSet

      51 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS2/result_set/

      Similar to the ones reported for Connection above. Synchronizing method getStatement in class ResultSet on connection_ eliminates them.

      ===============================
      java.sql.Statement

      51 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS2/statement/index.html

      Similar to the ones reported for Connection above. They are eliminated once the races reported below for PreparedStatement are eliminated.

      ===============================
      java.sql.PreparedStatement

      51 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS2/prepared_statement/index.html

      Similar to the ones reported for Connection above. Synchronizing 10 methods in class PreparedStatement on connection_ eliminates them:

      boolean execute(String sql)
      java.sql.ResultSet executeQuery(String sql)
      int executeUpdate(String sql)
      boolean execute(String sql, int autoGeneratedKeys)
      boolean execute(String sql, String[] columnNames)
      boolean execute(String sql, int[] columnIndexes)
      int executeUpdate(String sql, int autoGeneratedKeys)
      int executeUpdate(String sql, String[] columnNames)
      int executeUpdate(String sql, int[] columnIndexes)
      void setURL(int parameterIndex, java.net.URL x)

      ===============================
      java.sql.DatabaseMetaData

      3 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS2/database_metadata/index.html

      All are on field deferredException_ of class Agent, due to lack of synchronization on connection_ in 34 methods in class DatabaseMetaData:

      boolean supportsSavepoints()
      String getURL()
      String getUserName()
      String getDatabaseProductName()
      String getDatabaseProductVersion()
      String getDriverName()
      String getDriverVersion()
      boolean supportsMixedCaseIdentifiers()
      boolean supportsMixedCaseQuotedIdentifiers()
      String getIdentifierQuoteString()
      boolean supportsColumnAliasing()
      boolean nullPlusNonNullIsNull()
      boolean supportsTableCorrelationNames()
      boolean supportsLikeEscapeClause()
      boolean supportsNonNullableColumns()
      boolean supportsMinimumSQLGrammar()
      boolean supportsANSI92EntryLevelSQL()
      boolean supportsSubqueriesInExists()
      boolean supportsSubqueriesInIns()
      boolean supportsSubqueriesInQuantifieds()
      boolean supportsCorrelatedSubqueries()
      java.sql.Connection getConnection()
      boolean supportsNamedParameters()
      boolean supportsMultipleOpenResults()
      boolean supportsGetGeneratedKeys()
      boolean supportsResultSetHoldability(int holdability)
      int getResultSetHoldability()
      int getDatabaseMajorVersion()
      int getDatabaseMinorVersion()
      int getJDBCMajorVersion()
      int getJDBCMinorVersion()
      int getSQLStateType()
      boolean locatorsUpdateCopy()
      boolean supportsStatementPooling()

      The races arise because all these methods call checkForClosedConnection defined in the same class which in turn calls checkForDeferredExceptions in class Agent:

      void checkForDeferredExceptions() throws SqlException {
      if (deferredException_ != null)

      { SqlException temp = deferredException_; deferredException_ = null; throw temp; }

      }

      These races can be fixed by simply removing the call to checkForClosedConnection in most of the above methods, instead of adding synchronization to them. In particular, this method doesn't call it:

      // JDBC signature also does not throw SqlException, so we don't check for
      // closed connection.
      public int getDriverMajorVersion()

      { return Version.getMajorVersion(); }

      Then why does a trivial method like this one call it?

      public String getIdentifierQuoteString() throws SqlException

      { checkForClosedConnection(); return "\""; }

      Most of the above methods are trivial, simply returning a constant value. Do you really need to call checkForClosedConnection in them?

      ===============================
      java.sql.CallableStatement

      3 races:
      http://suif.stanford.edu/~mhn/db-derby-10.1.1.0-src/classes/PASS2/callable_statement/index.html

      All are on field deferredException_ of class Agent, due to lack of synchronization on connection_ in 52 methods in class CallableStatement (one of which is shown below):

      public java.net.URL getURL(String parameterName) throws SqlException {
      if (agent_.loggingEnabled())

      { agent_.logWriter_.traceEntry(this, "getURL", parameterName); }

      super.checkForClosedStatement();
      throw new SqlException(agent_.logWriter_, "JDBC 3 method called - not yet supported");
      }

      The above method calls checkForClosedStatement in class Statement which in turn calls checkForDeferredExceptions in class Agent.

        Activity

        Hide
        Mike Matrigali added a comment -

        Triaged July 2, 2009: assigned normal urgency.

        A tool was run that marked possible problems, but no repro or report of an actual problem.

        Show
        Mike Matrigali added a comment - Triaged July 2, 2009: assigned normal urgency. A tool was run that marked possible problems, but no repro or report of an actual problem.

          People

          • Assignee:
            Unassigned
            Reporter:
            Mayur Naik
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development