Thread: Deadlock while using getNotifications() and Statement.executeQuery()

Deadlock while using getNotifications() and Statement.executeQuery()

From
Joao Rui Leal
Date:
Hello!

I'm using more than one thread in my application. In one thread I listen to notifications from the database and another
onechecks if the connection is alive by doing a "select 1". 
Sometimes I get a deadlock!!!
I'm using postgresql-8.2-508.jdbc4.jar.

The thread dump gives me this:

Java stack information for the threads listed above:
===================================================
"DB connection select 1":
        at org.postgresql.core.v3.ProtocolConnectionImpl.setTransactionState(ProtocolConnectionImpl.java:191)
        - waiting to lock <0xb1a00f88> (a org.postgresql.core.v3.ProtocolConnectionImpl)
        at org.postgresql.core.v3.QueryExecutorImpl.receiveRFQ(QueryExecutorImpl.java:1654)
        at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:1410)
        at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:193)
        - locked <0xb1a033e0> (a org.postgresql.core.v3.QueryExecutorImpl)
        at org.postgresql.jdbc2.AbstractJdbc2Statement.execute(AbstractJdbc2Statement.java:452)
        at org.postgresql.jdbc2.AbstractJdbc2Statement.executeWithFlags(AbstractJdbc2Statement.java:337)
        at org.postgresql.jdbc2.AbstractJdbc2Statement.executeQuery(AbstractJdbc2Statement.java:236)
        at database.DatabaseManager$SelectRunnable.run(DatabaseManager.java:483)
        at java.lang.Thread.run(Thread.java:619)
"Database tables listener":
        at org.postgresql.core.v3.QueryExecutorImpl.processNotifies(QueryExecutorImpl.java:547)
        - waiting to lock <0xb1a033e0> (a org.postgresql.core.v3.QueryExecutorImpl)
        at org.postgresql.core.v3.ProtocolConnectionImpl.getNotifications(ProtocolConnectionImpl.java:68)
        - locked <0xb1a00f88> (a org.postgresql.core.v3.ProtocolConnectionImpl)
        at org.postgresql.jdbc2.AbstractJdbc2Connection.getNotifications(AbstractJdbc2Connection.java:1013)
        at database.DbListenerHandler$DbListener.run(DbListenerHandler.java:134)
        at java.lang.Thread.run(Thread.java:619)

Found 1 deadlock.

I've marked the places where the deadlock occurs with  "\\<<<<<<<<<<<".

In the "DB connection select 1" thread I have:
====================================
success = false;
Statement stmt = null;
try {
   int i = 0;
   stmt = getCon().createStatement();
   ResultSet rs = stmt.executeQuery("SELECT 1");\\<<<<<<<<<<<

   if (rs.next()) {
      i = rs.getInt(1);
   }
    if (i == 1) {
      success = true;
   }
 } catch (SQLException ex) {
 } finally {
   if (stmt != null) {
      try {
         stmt.close();
       } catch (SQLException ex) {
       }
   }
 }

====================================

And in the "Database tables listener" thread I have:
====================================
while (...) {
   try {
      PGNotification notifications[] =pgconn.getNotifications();\\<<<<<<<<<<<
      if (notifications != null) {
    ....
      }
      Thread.sleep(300);
   } catch (SQLException sqle) {
    ....
   } catch (InterruptedException ie) {
    ....
   }
}
====================================

Is this a bug in jdbc-postgres?

Joao Leal

Re: Deadlock while using getNotifications() and Statement.executeQuery()

From
Joao Rui Leal
Date:
I did a workaround/hack to fix the problem, but there should be some better
way to fix this. I had to make sure that the locking order in
getNotifications() is same as in executeQuery(), but that meant exposing the
QueryExecutor in the ProtocolConnection (the QueryExecutorImpl is saved as
private variable inside ProtocolConnectionImpl). So I had to make the
following changes (it's not pretty, I know...):

=====================================
org/postgresql/jdbc2/AbstractJdbc2Connection.java

1012,1014c1012,1016
<         // Backwards-compatibility hand-holding.
<         PGNotification[] notifications = protoConnection.getNotifications();
<         return (notifications.length == 0 ? null : notifications);
---
>         synchronized(protoConnection.getQueryExecutorImpl()) {
>            // Backwards-compatibility hand-holding.
>            PGNotification[] notifications =
protoConnection.getNotifications();
>            return (notifications.length == 0 ? null : notifications);
>         }

=====================================

org/postgresql/core/v2/ProtocolConnectionImpl.java
and
org/postgresql/core/v3/ProtocolConnectionImpl.java

>     public QueryExecutorImpl getQueryExecutorImpl() {
>         return executor;
>     }
>
=====================================
org/postgresql/core/ProtocolConnection.java

13a14
> import org.postgresql.core.QueryExecutor;
134a136,141
>
>     /**
>      * Used only to lock the executor object
>      * @return
>      */
>     public QueryExecutor getQueryExecutorImpl();

With these changes the problem was solved, but not in the best way.

Any comments?



On Monday 24 March 2008, Joao Rui Leal wrote:
> Hello!
>
> I'm using more than one thread in my application. In one thread I listen to
> notifications from the database and another one checks if the connection is
> alive by doing a "select 1". Sometimes I get a deadlock!!!
> I'm using postgresql-8.2-508.jdbc4.jar.
>
> The thread dump gives me this:
>
> Java stack information for the threads listed above:
> ===================================================
> "DB connection select 1":
>         at
> org.postgresql.core.v3.ProtocolConnectionImpl.setTransactionState(ProtocolC
>onnectionImpl.java:191) - waiting to lock <0xb1a00f88> (a
> org.postgresql.core.v3.ProtocolConnectionImpl) at
> org.postgresql.core.v3.QueryExecutorImpl.receiveRFQ(QueryExecutorImpl.java:
>1654) at
> org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.j
>ava:1410) at
> org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:193
>) - locked <0xb1a033e0> (a org.postgresql.core.v3.QueryExecutorImpl) at
> org.postgresql.jdbc2.AbstractJdbc2Statement.execute(AbstractJdbc2Statement.
>java:452) at
> org.postgresql.jdbc2.AbstractJdbc2Statement.executeWithFlags(AbstractJdbc2S
>tatement.java:337) at
> org.postgresql.jdbc2.AbstractJdbc2Statement.executeQuery(AbstractJdbc2State
>ment.java:236) at
> database.DatabaseManager$SelectRunnable.run(DatabaseManager.java:483) at
> java.lang.Thread.run(Thread.java:619)
> "Database tables listener":
>         at
> org.postgresql.core.v3.QueryExecutorImpl.processNotifies(QueryExecutorImpl.
>java:547) - waiting to lock <0xb1a033e0> (a
> org.postgresql.core.v3.QueryExecutorImpl) at
> org.postgresql.core.v3.ProtocolConnectionImpl.getNotifications(ProtocolConn
>ectionImpl.java:68) - locked <0xb1a00f88> (a
> org.postgresql.core.v3.ProtocolConnectionImpl) at
> org.postgresql.jdbc2.AbstractJdbc2Connection.getNotifications(AbstractJdbc2
>Connection.java:1013) at
> database.DbListenerHandler$DbListener.run(DbListenerHandler.java:134) at
> java.lang.Thread.run(Thread.java:619)
>
> Found 1 deadlock.
>
> I've marked the places where the deadlock occurs with  "\\<<<<<<<<<<<".
>
> In the "DB connection select 1" thread I have:
> ====================================
> success = false;
> Statement stmt = null;
> try {
>    int i = 0;
>    stmt = getCon().createStatement();
>    ResultSet rs = stmt.executeQuery("SELECT 1");\\<<<<<<<<<<<
>
>    if (rs.next()) {
>       i = rs.getInt(1);
>    }
>     if (i == 1) {
>       success = true;
>    }
>  } catch (SQLException ex) {
>  } finally {
>    if (stmt != null) {
>       try {
>          stmt.close();
>        } catch (SQLException ex) {
>        }
>    }
>  }
>
> ====================================
>
> And in the "Database tables listener" thread I have:
> ====================================
> while (...) {
>    try {
>       PGNotification notifications[]
> =pgconn.getNotifications();\\<<<<<<<<<<< if (notifications != null) {
>     ....
>       }
>       Thread.sleep(300);
>    } catch (SQLException sqle) {
>     ....
>    } catch (InterruptedException ie) {
>     ....
>    }
> }
> ====================================
>
> Is this a bug in jdbc-postgres?
>
> Joao Leal
>
> -
> Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-jdbc



--
Engº de Processo
// Ciengis - Sistemas de Controlo Avançado, Lda
IPN, Rua Pedro Nunes
3030-199 Coimbra
PORTUGAL

Tel/Fax: +351 239 700 353 / 301
Telm.: +351 934 699 919

www.ciengis.com

Re: Deadlock while using getNotifications() and Statement.executeQuery()

From
Kris Jurka
Date:

On Tue, 25 Mar 2008, Joao Rui Leal wrote:

> I did a workaround/hack to fix the problem, but there should be some better
> way to fix this. I had to make sure that the locking order in
> getNotifications() is same as in executeQuery(), but that meant exposing the
> QueryExecutor in the ProtocolConnection (the QueryExecutorImpl is saved as
> private variable inside ProtocolConnectionImpl). So I had to make the
> following changes (it's not pretty, I know...):

I don't see why you need access to the Impl version.  Isn't "synchronized
(getQueryExecutor())" around AbstractJdbc2Connection's
protoConnection.getNotifications() sufficient?

Still that's not a real clean/understandable design.  Perhaps instead
processNotifies() should be added to the public QueryExecutor interface
and then AbstractJdbc2Connection can call processNotifies itself so that
fetching notifications from protoConnection doesn't require any
interaction with the QueryExecutor.

Kris Jurka

Re: Deadlock while using getNotifications() and Statement.executeQuery()

From
Joao Rui Leal
Date:
On Tuesday 25 March 2008, Kris Jurka wrote:
> On Tue, 25 Mar 2008, Joao Rui Leal wrote:
> > I did a workaround/hack to fix the problem, but there should be some
> > better way to fix this. I had to make sure that the locking order in
> > getNotifications() is same as in executeQuery(), but that meant exposing
> > the QueryExecutor in the ProtocolConnection (the QueryExecutorImpl is
> > saved as private variable inside ProtocolConnectionImpl). So I had to
> > make the following changes (it's not pretty, I know...):
>
> I don't see why you need access to the Impl version.  Isn't "synchronized
> (getQueryExecutor())" around AbstractJdbc2Connection's
> protoConnection.getNotifications() sufficient?

I haven't tested it yet but you seem to be right. I didn't know there was such
a method (1st time going through the API). I guess if it returns the
org.postgresql.core.v3.QueryExecutorImpl object then the locking will be in
the same order as in executeQuery().

>
> Still that's not a real clean/understandable design.  Perhaps instead
> processNotifies() should be added to the public QueryExecutor interface
> and then AbstractJdbc2Connection can call processNotifies itself so that
> fetching notifications from protoConnection doesn't require any
> interaction with the QueryExecutor.

I agree.

Joao Leal

>
> Kris Jurka

Re: Deadlock while using getNotifications() and Statement.executeQuery()

From
Kris Jurka
Date:
Joao Rui Leal wrote:
> On Tuesday 25 March 2008, Kris Jurka wrote:
>> Still that's not a real clean/understandable design.  Perhaps instead
>> processNotifies() should be added to the public QueryExecutor interface
>> and then AbstractJdbc2Connection can call processNotifies itself so that
>> fetching notifications from protoConnection doesn't require any
>> interaction with the QueryExecutor.
>
> I agree.
>

I've applied the attached patch to CVS back to the 8.1 driver.  The 8.0
driver does not have this problem because it doesn't allow notification
retrieval without executing a query.

Thanks for the report and diagnosis.

Kris Jurka
Index: org/postgresql/core/QueryExecutor.java
===================================================================
RCS file: /cvsroot/jdbc/pgjdbc/org/postgresql/core/QueryExecutor.java,v
retrieving revision 1.41
diff -c -r1.41 QueryExecutor.java
*** org/postgresql/core/QueryExecutor.java    8 Jan 2008 06:56:27 -0000    1.41
--- org/postgresql/core/QueryExecutor.java    1 Apr 2008 07:09:38 -0000
***************
*** 170,175 ****
--- 170,184 ----
       */
      Query createParameterizedQuery(String sql); // Parsed for parameter placeholders ('?')

+     /**
+      * Prior to attempting to retrieve notifications, we need to pull
+      * any recently received notifications off of the network buffers.
+      * The notification retrieval in ProtocolConnection cannot do this
+      * as it is prone to deadlock, so the higher level caller must be
+      * responsible which requires exposing this method.
+      */
+     void processNotifies() throws SQLException;
+
      //
      // Fastpath interface.
      //
Index: org/postgresql/core/v2/ProtocolConnectionImpl.java
===================================================================
RCS file: /cvsroot/jdbc/pgjdbc/org/postgresql/core/v2/ProtocolConnectionImpl.java,v
retrieving revision 1.11
diff -c -r1.11 ProtocolConnectionImpl.java
*** org/postgresql/core/v2/ProtocolConnectionImpl.java    8 Jan 2008 06:56:27 -0000    1.11
--- org/postgresql/core/v2/ProtocolConnectionImpl.java    1 Apr 2008 07:09:39 -0000
***************
*** 62,68 ****
      }

      public synchronized PGNotification[] getNotifications() throws SQLException {
-         executor.processNotifies();
          PGNotification[] array = (PGNotification[])notifications.toArray(new PGNotification[notifications.size()]);
          notifications.clear();
          return array;
--- 62,67 ----
Index: org/postgresql/core/v3/ProtocolConnectionImpl.java
===================================================================
RCS file: /cvsroot/jdbc/pgjdbc/org/postgresql/core/v3/ProtocolConnectionImpl.java,v
retrieving revision 1.12
diff -c -r1.12 ProtocolConnectionImpl.java
*** org/postgresql/core/v3/ProtocolConnectionImpl.java    8 Jan 2008 06:56:27 -0000    1.12
--- org/postgresql/core/v3/ProtocolConnectionImpl.java    1 Apr 2008 07:09:39 -0000
***************
*** 65,71 ****
      }

      public synchronized PGNotification[] getNotifications() throws SQLException {
-         executor.processNotifies();
          PGNotification[] array = (PGNotification[])notifications.toArray(new PGNotification[notifications.size()]);
          notifications.clear();
          return array;
--- 65,70 ----
Index: org/postgresql/jdbc2/AbstractJdbc2Connection.java
===================================================================
RCS file: /cvsroot/jdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2Connection.java,v
retrieving revision 1.48
diff -c -r1.48 AbstractJdbc2Connection.java
*** org/postgresql/jdbc2/AbstractJdbc2Connection.java    19 Feb 2008 06:12:24 -0000    1.48
--- org/postgresql/jdbc2/AbstractJdbc2Connection.java    1 Apr 2008 07:09:39 -0000
***************
*** 1022,1027 ****
--- 1022,1028 ----

      public PGNotification[] getNotifications() throws SQLException
      {
+         getQueryExecutor().processNotifies();
          // Backwards-compatibility hand-holding.
          PGNotification[] notifications = protoConnection.getNotifications();
          return (notifications.length == 0 ? null : notifications);