ActiveMQ
  1. ActiveMQ
  2. AMQ-3760

Deadlock on Java Client when onMessage executes a synchronized method.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.5.1
    • Fix Version/s: AGING_TO_DIE
    • Component/s: JMS client
    • Labels:
    • Environment:

      java version "1.6.0_30"
      Java(TM) SE Runtime Environment (build 1.6.0_30-b12)
      Java HotSpot(TM) Server VM (build 20.5-b03, mixed mode)

      OS: SunOs 10/06

      Description

      Create a class with two synchronized methods synchMethod1 and synchMethod2.
      syncMethod1 is invoked inside a messageListener object and only prints a message on stdout.
      syncMethod2 creates a new MessageConsumer with a new instance of MessageListener. All the MessageConsumers created by synchMethod2 share the same session.
      All the consumers listen on the same Topic named "myDest".

      Create an infinite cycle that:
      1) send a message on Topic "myDest"
      2) invoke syncMethod2
      3) waits 10ms

      In attach the unit test source.

      As result, a deadlock is created between the "ActiveMQ Session Task" thread entering in syncMethod1 having the ActiveMessageConsumer's mutex and my other thread executing the syncMethod2 when the SimplePriorityMessageDispatchChannel.stop() method is invoked.

      This means that it's not possible to create or change MessageListeners on a running session, even if the code seems to handle this case stopping and restarting the session.

      1. ActiveMQDeadlock.zip
        2 kB
        Alessandro Monguzzi

        Activity

        Alessandro Monguzzi created issue -
        Hide
        Alessandro Monguzzi added a comment -

        Deadlock trace and unit test

        Show
        Alessandro Monguzzi added a comment - Deadlock trace and unit test
        Alessandro Monguzzi made changes -
        Field Original Value New Value
        Attachment ActiveMQDeadlock.zip [ 12517397 ]
        Hide
        Timothy Bish added a comment -

        This sort of use of sessions, producers and consumers is discouraged by the spec:

        "If a client desires to have one thread produce messages while others consume them, the client should use a separate session for its producing thread.

        Once a connection has been started, any session with one or more registered message listeners is dedicated to the thread of control that delivers messages to it. It is erroneous for client code to use this session or any of its constituent objects from another thread of control. The only exception to this rule is the use of the session or connection close method. "

        Show
        Timothy Bish added a comment - This sort of use of sessions, producers and consumers is discouraged by the spec: "If a client desires to have one thread produce messages while others consume them, the client should use a separate session for its producing thread. Once a connection has been started, any session with one or more registered message listeners is dedicated to the thread of control that delivers messages to it. It is erroneous for client code to use this session or any of its constituent objects from another thread of control. The only exception to this rule is the use of the session or connection close method. "
        Hide
        Alessandro Monguzzi added a comment -

        In the unit test above I used a dedicated session for the producer. Only the consumers share a session.

        So is it not possible to add a new Consumer to an already running session? Can't I change the messageListener for a running consumer?

        If so, I think that the MessageConsumer.setMessageListener() should throw an Exception, not stopping and restarting the session.

        Show
        Alessandro Monguzzi added a comment - In the unit test above I used a dedicated session for the producer. Only the consumers share a session. So is it not possible to add a new Consumer to an already running session? Can't I change the messageListener for a running consumer? If so, I think that the MessageConsumer.setMessageListener() should throw an Exception, not stopping and restarting the session.
        Hide
        Timothy Bish added a comment -

        The spec has this to say about changing the message listener on a consumer:

        "The effect of calling MessageConsumer.setMessageListener while messages are being consumed by an existing listener or the consumer is being used to consume messages synchronously is undefined."

        Show
        Timothy Bish added a comment - The spec has this to say about changing the message listener on a consumer: "The effect of calling MessageConsumer.setMessageListener while messages are being consumed by an existing listener or the consumer is being used to consume messages synchronously is undefined."
        Hide
        Alessandro Monguzzi added a comment -

        Ok, I understant I cannot change the MessageListener for a running consumer (even if I think that undefined should mean I'm not sure that I will not miss messages in the meanwhile, not that the application runs into deadlock).

        It remains open the point: can I add a new consumer on a running session? The code seems to handle this scenario. Else an exception should be thrown.

        Show
        Alessandro Monguzzi added a comment - Ok, I understant I cannot change the MessageListener for a running consumer (even if I think that undefined should mean I'm not sure that I will not miss messages in the meanwhile, not that the application runs into deadlock). It remains open the point: can I add a new consumer on a running session? The code seems to handle this scenario. Else an exception should be thrown.
        Hide
        Timothy Bish added a comment -

        From the spec:

        "Once a connection has been started, any session with one or more registered message listeners is dedicated to the thread of control that delivers messages to it. It is erroneous for client code to use this session or any of its constituent objects from another thread of control. The only exception to this rule is the use of the session or connection close method. "

        In your test case the first addition of the consumer succeeds which is correct, after that the session has a running consumer and is subject to the rule that its invalid for any other thread to call a session method other than close, which your code does by attempting to create a new consumer from the producer thread. As per the spec the only Session method that you can safely call from the producer thread is close() after you create the first consumer as its now dedicated to the thread that is delivering messages to the consumer.

        Show
        Timothy Bish added a comment - From the spec: "Once a connection has been started, any session with one or more registered message listeners is dedicated to the thread of control that delivers messages to it. It is erroneous for client code to use this session or any of its constituent objects from another thread of control. The only exception to this rule is the use of the session or connection close method. " In your test case the first addition of the consumer succeeds which is correct, after that the session has a running consumer and is subject to the rule that its invalid for any other thread to call a session method other than close, which your code does by attempting to create a new consumer from the producer thread. As per the spec the only Session method that you can safely call from the producer thread is close() after you create the first consumer as its now dedicated to the thread that is delivering messages to the consumer.
        Hide
        Alessandro Monguzzi added a comment -

        Ok, so your answer to the question: "can I add a consumer on a running session?" is no.

        At this point, why the ActiveMQMessageConsumer.setMessageListener stops and restarts the session instead of throwing an exception? If the operation is invalid, this behaviour is error prone and inefficient. Please note that in this scenario it's possible to block forever every other consumer linked to the same session, also those created before the session start.
        Since this code works correctly if no lock is involved, I believe we have to resolve the deadlock, not to drop the entire feature.

        Show
        Alessandro Monguzzi added a comment - Ok, so your answer to the question: "can I add a consumer on a running session?" is no. At this point, why the ActiveMQMessageConsumer.setMessageListener stops and restarts the session instead of throwing an exception? If the operation is invalid, this behaviour is error prone and inefficient. Please note that in this scenario it's possible to block forever every other consumer linked to the same session, also those created before the session start. Since this code works correctly if no lock is involved, I believe we have to resolve the deadlock, not to drop the entire feature.
        Hide
        Timothy Bish added a comment -

        Actually, the answer to the question: "can I add a consumer on a running session?" is as it is in most cases, it depends. In many cases you can happily add a new consumer and even set a new message listener to a running consumer without problems, only in the tangled cases of synchronized methods cases you've created does this run into problems and that's why the spec makes these notes about the use of sessions and consumes by threads other than the thread that is delivering to the session with a running consumer.

        The code is working within the bounds of the spec. If you have some ideas on how to improve things such that it meets your use case while still obeying all the various rules of the JMS specification we welcome your patches and test cases.

        Show
        Timothy Bish added a comment - Actually, the answer to the question: "can I add a consumer on a running session?" is as it is in most cases, it depends. In many cases you can happily add a new consumer and even set a new message listener to a running consumer without problems, only in the tangled cases of synchronized methods cases you've created does this run into problems and that's why the spec makes these notes about the use of sessions and consumes by threads other than the thread that is delivering to the session with a running consumer. The code is working within the bounds of the spec. If you have some ideas on how to improve things such that it meets your use case while still obeying all the various rules of the JMS specification we welcome your patches and test cases.
        Hide
        Timothy Bish added a comment -

        Working as designed.

        Show
        Timothy Bish added a comment - Working as designed.
        Timothy Bish made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Resolution Not A Problem [ 8 ]
        Hide
        Alessandro Monguzzi added a comment -

        I disagree with the closure of this issue.

        The spec cannot allow a deadlock and a synchronized method is not an uncommon case. Either a method can be called or it can't, the behaviour cannot be unpredictable.
        If none Session method can be called (apart from close) when a Session is running, then I could not call the createObjectMessage() method also. I cannot believe that.

        I would leave this bug open and explore a solution; closing the bug in this moment is like hiding the issue.

        Show
        Alessandro Monguzzi added a comment - I disagree with the closure of this issue. The spec cannot allow a deadlock and a synchronized method is not an uncommon case. Either a method can be called or it can't, the behaviour cannot be unpredictable. If none Session method can be called (apart from close) when a Session is running, then I could not call the createObjectMessage() method also. I cannot believe that. I would leave this bug open and explore a solution; closing the bug in this moment is like hiding the issue.
        Alessandro Monguzzi made changes -
        Resolution Not A Problem [ 8 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Timothy Bish made changes -
        Fix Version/s AGING_TO_DIE [ 12315631 ]
        Timothy Bish made changes -
        Status Reopened [ 4 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Alessandro Monguzzi
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development