Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8350368

debug agent clearStep() not always called in a thread safe manner

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: P4 P4
    • 25
    • 25
    • core-svc
    • None

      There are multiple threads that can end up calling clearStep(), so we need to be careful about races. There are two main paths into clearStep.

      - The first is through stepControl_endStep. It always grabs the handler_lock and stepControl_lock.
      - The 2nd is through through stepControl_clearRequest, which grabs no locks itself, but there are some already grabbed when it is called. It should really grab the stepControl_lock, but it can't because that lock is suppose to be grabbed before threadControl_lock, which has already been grabbed. The proper order is handler_lock -> stepControl_lock -> threadControl_lock. So we need to fix locking for stepControl_clearRequest somehow.

      There are 3 paths into stepControl_clearRequest().

      - The first goes through clearThread via removeThread via removeResumed. This path always holds the handler_lock and threadControl_lock.
      - The 2nd goes through clearThread via removeThread via threadControl_onEventHandlerExit. This also holds the handler_lock and threadControl_lock.
      - The 3rd goes through clearThread via removeVThreads via threadControl_reset. This path always holds the handler_lock, but not threadControl_lock

      Holding the threadControl_lock here does not help us since stepControl_endStep does not grab that lock. Holding the eventHandler_lock does help since stepControl_endStep does grab it, but the 3rd path above does not grab the eventHandler_lock. I think the simplest fix here is to just make it do so. But that's using the eventHandler_lock to make stepControl code thread safe, when stepControl_lock should be used. Not exactly clean.

      So that means we need to require the caller of stepControl_endStep to do all the needed locking first. Basically add locking of stepControl_lock somewhere. Adding locking of stepControl_lock in clearThread is too late since we already hold the threadLock. It really needs to be pushed up the call chain of each of the above 3 paths to the point just before grabbing the threadLock. For example for the 2nd call path above we have:

      if (ei == EI_THREAD_END) {
          eventHandler_lock(); /* for proper lock order - see removeThread() call below */
          debugMonitorEnter(threadLock);

      So we need to add stepControl_lock() in between those too lines. We need to do similar for the 1st and 3rd paths also.

      It would be good to resolve this as part of or just after the rank locking support is done (JDK-8328866) since this is very lock order sensitive code, and the rank locking support will help detect errors.

            cjplummer Chris Plummer
            cjplummer Chris Plummer
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated: