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

["Peter Jones (JIRA)" <ji...@apache.org>]

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

                 Key: RIVER-145
                 URL: https://issues.apache.org/jira/browse/RIVER-145
             Project: River
          Issue Type: Bug
          Components: net_jini_lookup
    Affects Versions: jtsk_2.1
            Reporter: Peter Jones
            Priority: Minor


Bugtraq ID [6219020|http://bugs.sun.com/bugdatabase/view_bug.do?bug_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}}.

h4. ( Comments note: }

{noformat}
JoinManager {
  ServiceItem serviceItem
                - serviceID
                - service
                - attributesSets
}
{noformat}

-- 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,

{noformat}
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 ***
{noformat}

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.

{noformat}
-- 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)
{noformat}

h4. ( 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.


-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.