[RIVER-145] JoinManager synchronization on serviceItem should be reviewed, doc'd and fixed where appropriate

• Type: Bug
• Status: Resolved
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: jtsk_2.1
• Fix Version/s: River_3.0.0
• Component/s: net_jini_lookup
• Bugtraq ID: 6219020
• Assignee: Unassigned
• Reporter: Peter Jones
• Created: 27/Jul/07 22:35
• Updated: 05/Dec/15 08:04
• Resolved: 05/Dec/15 08:04

Description

Bugtraq ID 6219020

The object JoinManager.serviceItem and its fields are accessed (read
and written) in a number of threads/tasks in JoinManager. In most cases,
that access is controlled through synchronization on either serviceItem
or the JoinManager.joinSet object. But there are a few places where the
access to serviceItem fields is not synchronized at all.

The synchronization strategy and implementation for serviceItem should
be reviewed and corrected where necessary.

See the comments for a 'map' of serviceItem synchronization, and a
description of the synchronization strategy regarding serviceItem.

( Comments note: }

JoinManager {
  ServiceItem serviceItem
                - serviceID
                - service
                - attributesSets
}

– Synchronization Strategy for serviceItem –

serviceItem is instantiated in createJoinManager() when the JoinManager is
created and initialized. After instantiation, only the fields of serviceItem
are accessed by the various threads. Access to those fields is controlled by
synchronizing on either serviceItem itself, on joinSet, or on both.

• Access to the serviceID field is controlled by synchronizing on
  serviceItem itself.
• Access to the service and attributeSets fields is controlled by
  synchronizing on joinSet.

The reason that joinSet is not used to control access to all the fields of
serviceItem is because deadlock can occur if the sync on serviceItem in
ProxyRegTask.runAfter() is changed to a sync on joinSet. This deadlock
can occur because the TaskManager actually executes the runAfter() method
inside of a sync block on the TaskManager itself. Thus,

A. LUS-0 discovered 

DiscMgrListener.discovered() -- LUS-0
  sync(joinSet)
    addTask(RegisterTask-0)
      sync(taskList)
        taskList.add(RegisterTask-0)
        sync(taskMgr)
          taskMgr.add(ProxyRegTask-0) ---------------------| 
        }//end sync                                        |
      }//end sync                                          |
  }//end sync                                              |
                                                           |
                                       B.  TaskManager.run(ProxyRegTask-0)
                                         |-- sync(taskMgr) -- (lock-1)
C. LUS-1 discovered                      |     ProxyRegTask-0.runAfter()
                                         |      .
DiscMgrListener.discovered() -- LUS-1    |      .
  sync(joinSet) -- (lock-2) ----------|  |      .
    addTask(RegisterTask-1)           |-------- sync(joinSet) (wait on lock-2)
      sync(taskList)                     |      
        taskList.add(RegisterTask-1)     |
        sync(taskMgr) (wait on lock-1) --|       *** DEADLOCK ***

Thus, to avoid the above deadlock, runAfter() and ProxyReg.register() both
synchronize on serviceItem before accessing the serviceID field of
serviceItem. For access to the other fields of serviceItem, synchronizing
on joinSet seems to be sufficient; although, as the map below shows,
there are places where no synchronization occurs, but should be
considered.

-- Synchronization Map --

serviceItem field     access location           access type  synchronization
----------------- -----------------------      ----------- -----------------
a.) serviceID     ProxyRegTask.runAfter()      read        sync(serviceItem)
                  ProxyReg.register()          read write  sync(serviceItem)

b.) service       ProxyReg.register()          read write  sync(joinSet)
                                                           & sync(serviceItem)
                  DiscMgrListener.discovered() read        sync(joinSet)
                  replaceRegistrationDo()           write  sync(joinSet)
    ***           createJoinManager()               write  NO SYNC
                  (when serviceItem instantiated)

c.) attributeSets addAttributes()                   write  sync(joinSet)
                  setAttributes()                   write  sync(joinSet)
                  modifyAttributes()                write  sync(joinSet)
                  replaceRegistrationDo()           write  sync(joinSet)
    ***           createJoinManager()               write  NO SYNC
                  (when serviceItem instantiated)

( Suggested Fix note: )

Currently, the only place in JoinManager where access to the fields of
serviceItem are not synchronized is in createJoinManager(). In that
method, the serviceItem is created (and its fields are written to) when
the JoinManager itself is constructed and initialized. Because this occurs
during construction/initialization, before any tasks or threads that access
serviceItem have been started, it may not be necessary to synchronize
access to that new serviceItem in createJoinManager.

Thus, there may not currently be any sort of defect associated with
synchronization on serviceItem in JoinManager. This bug report can
then be viewed as a location to document any future defects or changes
related to that synchronization.