Qpid
  1. Qpid
  2. QPID-3807

Improve the thread safety of JMS session dispatcher

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.15
    • Fix Version/s: 0.15
    • Component/s: Java Client
    • Labels:
      None

      Description

      The access to the AMQSession#_dispatcher is not a thread safe and can result in thread safety issues.
      One of this issue is reported in QPID-3782.

        Activity

        Hide
        Keith Wall added a comment -

        It seems to me that the only explanation for QPID-3872 is a publication issue.

        At the minute, reads of members _dispatcher and _dispatcherThread are not thread safe and therefore a thread could read a stale values.

        It appears that is what has happened in QPID-3872. The dispatcher thread was running run(), it called #dispatch() which first checked whether the _dispatcher member is non-null. In our failing case, the _dispatcher member was null. This could not normally be the case as _dispatcher is assigned before the Dispatcher thread is created (in startDispatcherIfNecessary).

        I'm suggest changing _dispatcher and _dispatcherThread to volatile. The synchronisation within startDispatcherIfNecessary() should remain. The setters setDispatcherThread and setDispatcher are unused and should be removed.

        Show
        Keith Wall added a comment - It seems to me that the only explanation for QPID-3872 is a publication issue. At the minute, reads of members _dispatcher and _dispatcherThread are not thread safe and therefore a thread could read a stale values. It appears that is what has happened in QPID-3872 . The dispatcher thread was running run(), it called #dispatch() which first checked whether the _dispatcher member is non-null. In our failing case, the _dispatcher member was null. This could not normally be the case as _dispatcher is assigned before the Dispatcher thread is created (in startDispatcherIfNecessary). I'm suggest changing _dispatcher and _dispatcherThread to volatile. The synchronisation within startDispatcherIfNecessary() should remain. The setters setDispatcherThread and setDispatcher are unused and should be removed.
        Hide
        Keith Wall added a comment -

        Hi Robbie, could you review this commit please?

        Show
        Keith Wall added a comment - Hi Robbie, could you review this commit please?
        Hide
        Robbie Gemmell added a comment -

        Changes look good to me.

        Show
        Robbie Gemmell added a comment - Changes look good to me.

          People

          • Assignee:
            Robbie Gemmell
            Reporter:
            Alex Rudyy
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development