[jira] Commented: (RIVER-148) JoinManager.ProxyReg.fail synchronization may be wrong or may be able to simplify it

["Jim Hurley (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/RIVER-148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12516424 ] 

Jim Hurley commented on RIVER-148:
----------------------------------

Suggested Fix (btmurphy):
----------------------------------------------

public class JoinManager {
    ....
    private class ProxyReg {
        ....
        public void fail(Throwable e) {
            synchronized(JoinManager.this) { <----- suggested fix 1
            synchronized(joinSet) { <-------------- suggested fix 2
                if(bTerminated) {
                    return;
                } else {
                    ....
                }//endif
            }//end sync(this)
        }//end ProxyReg.fail
        ....
    }//end class ProxyReg
    ....
}//end class JoinManager

If suggested fix 2 is chosen, then all the places where JoinManager is
synchronized should be changed to also sync on joinSet. Upon doing that,
any further simplications of the code (such as combining sync blocks)
should also be performed. For example, in getJoinSet(),

    public ServiceRegistrar[] getJoinSet() {
        synchronized(this) {
            if(bTerminated) throw ....
        }//end sync
	synchronized(joinSet) {
            ....
	}//end sync(joinSet)
    }//end getJoinSet

could be changed to:

    public ServiceRegistrar[] getJoinSet() {
	synchronized(joinSet) {
            if(bTerminated) throw ....
            ....
	}//end sync(joinSet)
    }//end getJoinSet




> JoinManager.ProxyReg.fail synchronization may be wrong or may be able to simplify it
> ------------------------------------------------------------------------------------
>
>                 Key: RIVER-148
>                 URL: https://issues.apache.org/jira/browse/RIVER-148
>             Project: River
>          Issue Type: Bug
>          Components: net_jini_lookup
>    Affects Versions: jtsk_2.1
>            Reporter: Jim Hurley
>            Priority: Minor
>
> Bugtraq ID [6219149|http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6219149]
> Currently, ProxyReg.fail() synchronizes on 'this', which is the current
> instance of ProxyReg. Within that sync block, it checks to see if the
> JoinManager has been terminated. The check for termination is similar
> to checks performed in a number of other places in JoinManager, except
> that all the other checks are performed while synchronized on the
> JoinManager itself, not a particular proxyReg. The fail() method should
> probably be changed to sync on JoinManager.this, rather than on this.
> Additionally, the only situation in which synchronization is performed on 
> the JoinManager itself is when the JoinManager's terminated status is
> checked. It seems like synchronization could be simplified somewhat if
> those sync blocks were changed to synchronize on joinSet instead of on
> JoinManager.
> Could result in a race condition.

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