commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marco Speranza <marcospera...@apache.org>
Subject Re: [Graph] Test problems after last commit
Date Sat, 03 Mar 2012 14:29:26 GMT
Ciao


> rue, but *all* methods executions (synchronized and not) are
> performed inside the handler, contained in the proxy instance anyway.
> 
> So an user cannot use the same "lock"  in order to synchronize a block
> like this:

Let's give you an example.

here is our handler implementation, and imagine that the 'lock' is not export outside:

===
synchronized ( this.lock )
{
	try
	{
		return method.invoke( checkedToBeSynchronized, args );
	}
	catch ( InvocationTargetException e )
	{
		throw e.getTargetException();
	}
}
===

here is a possible user implementation:

- Main class create a synchronized graph instance: 

    MutableGraph g = CommonsGraph.synchronize( new UndirectMutableGraph.... );
  
- Thread 1 loops over the vertices:
synchronized(g) {
    for ( BaseLabeledVertex v2 : g.getVertices() )
    {
        // do somethings
    }
 }

- Thread 2, insert a new vertex:
for ( int i = start; i < end; i++ )
{
	BaseLabeledVertex v = new BaseLabeledVertex( valueOf( i ) );
	g.addVertex( v );
}


in that example thread, probably you would have a ConcurrentModificationException because:

1) Therad 1 uses a lock 'g' for the Iterator returned by g.getVertices()   
2) Thread 2 uses its own lock 'this.lock' stored into Handler class.

in this situation our data structure is *not* thread safe.

The only way to synchronize the data structure is that:

-Thread 2:
for ( int i = start; i < end; i++ )
{
   synchronized(g) {
	  BaseLabeledVertex v = new BaseLabeledVertex( valueOf( i ) );
	  g.addVertex( v );
  }
}

but this IMHO  is a wrong way to fix the problem, and moreover with this workaround we don't
need to 
use CommonGraph.synchronize() method, isn't it?


> I have not clear which benefit you want to obtain with that - looks at
> Collections#synchronized* methods and see that the separation of
> concerns in clear.

you are right maybe use an external lock is not useful. 


> Sure that we can, the only issue we have is that GroboUtils is not in
> the central repo, adding the external repository would make not easy
> having the component released.

unfortunately GroboUtil was the only lib that I found yesterday. Any suggestions to some other
libs?

all the best  ;-) 

--
Marco Speranza <marcosperanza@apache.org>
Google Code: http://code.google.com/u/marco.speranza79/

Il giorno 03/mar/2012, alle ore 14:34, Simone Tripodi ha scritto:

> Hola Marco,
> 
>> my doubt is this:  opening a synchronized block into handler implementation on 'this',
 in my opinion is not enough, because the method "CommonsGraph.synchronize()" returns a instance
of the Proxy and not an instance of handler.
> 
> true, but *all* methods executions (synchronized and not) are
> performed inside the handler, contained in the proxy instance anyway.
> 
> So an user cannot use the same "lock"  in order to synchronize a block
> like this:
> 
> ====
> Graph g = CommonsGraph.synchronize(g_);
>   ...
> synchronized(g) {
>     for ( BaseLabeledVertex v2 : g.getVertices() )
>     {
>         // do somethings
>     }
> }
> ====
> 
> I am not sure we can do much more of what we've already done, for that
> situation: the snippet below mixes the *data structure*
> synchronization with the business logic performed outside.
> 
>> So my idea is to open synchronized block inside handler implementation using the
same Proxy instance that the method 'CommonsGraph.synchronize' will return.
>> Furthermore I'd like to modify the API by adding also the possibility for the user
to use an your own lock, in that way
>> 
>> CommonsGraph.synchronize( G graph, Object lock )
> 
> I have not clear which benefit you want to obtain with that - looks at
> Collections#synchronized* methods and see that the separation of
> concerns in clear.
> 
>> Finally only one question. I think that we should add some tests in order to check
the correct implementation of our multithrading implementation.
>> Do you think that we can use an external library in test scope?
> 
> Sure that we can, the only issue we have is that GroboUtils is not in
> the central repo, adding the external repository would make not easy
> having the component released.
> 
> best,
> -Simo
> 
> http://people.apache.org/~simonetripodi/
> http://simonetripodi.livejournal.com/
> http://twitter.com/simonetripodi
> http://www.99soft.org/
> 
> 
> 
> On Sat, Mar 3, 2012 at 2:13 PM, Marco Speranza <marcosperanza@apache.org> wrote:
>> Morning Simo,
>> 
>> my doubt is this:  opening a synchronized block into handler implementation on 'this',
 in my opinion is not enough, because the method "CommonsGraph.synchronize()" returns a instance
of the Proxy and not an instance of handler.
>> So an user cannot use the same "lock"  in order to synchronize a block like this:
>> 
>> ====
>> Graph g = CommonsGraph.synchronize(g_);
>>    ...
>>  synchronized(g) {
>>      for ( BaseLabeledVertex v2 : g.getVertices() )
>>      {
>>          // do somethings
>>      }
>>  }
>> ====
>> 
>> So my idea is to open synchronized block inside handler implementation using the
same Proxy instance that the method 'CommonsGraph.synchronize' will return.
>> Furthermore I'd like to modify the API by adding also the possibility for the user
to use an your own lock, in that way
>> 
>> CommonsGraph.synchronize( G graph, Object lock )
>> 
>> WDYT?
>> 
>> Finally only one question. I think that we should add some tests in order to check
the correct implementation of our multithrading implementation.
>> Do you think that we can use an external library in test scope?
>> 
>> 
>> have a nice day
>> 
>> 
>> --
>> Marco Speranza <marcosperanza@apache.org>
>> Google Code: http://code.google.com/u/marco.speranza79/
>> 
>> Il giorno 03/mar/2012, alle ore 13:50, Simone Tripodi ha scritto:
>> 
>>> Good morning Marco,
>>> 
>>> I had the chance to have a deeper look at your yesterday's night work
>>> and think your additions are good improvements - I just wonder if we
>>> can replace the lock object with the handler itself, referencing
>>> `this` instead.
>>> 
>>> Thoughts?
>>> 
>>> best and have a nice WE,
>>> -Simo
>>> 
>>> http://people.apache.org/~simonetripodi/
>>> http://simonetripodi.livejournal.com/
>>> http://twitter.com/simonetripodi
>>> http://www.99soft.org/
>>> 
>>> 
>>> 
>>> On Sat, Mar 3, 2012 at 1:56 AM, Simone Tripodi <simonetripodi@apache.org>
wrote:
>>>> I saw the commit, please read comments inline.
>>>> best,
>>>> -Simo
>>>> 
>>>> http://people.apache.org/~simonetripodi/
>>>> http://simonetripodi.livejournal.com/
>>>> http://twitter.com/simonetripodi
>>>> http://www.99soft.org/
>>>> 
>>>> 
>>>> 
>>>> On Sat, Mar 3, 2012 at 1:40 AM, Marco Speranza <marcosperanza@apache.org>
wrote:
>>>>> Hi I fixed the problem using proxy/handler.
>>>>> 
>>>>> I put also a couple of tests. For do that I insert a test scoped dependency
to a library net.sourceforge.groboutils.groboutils-core
>>>>> 
>>>>> Ciao
>>>>> 
>>>>> --
>>>>> Marco Speranza <marcosperanza@apache.org>
>>>>> Google Code: http://code.google.com/u/marco.speranza79/
>>>>> 
>>>>> Il giorno 03/mar/2012, alle ore 01:29, Simone Tripodi ha scritto:
>>>>> 
>>>>>> yes, what I didn't understand is how you would like to manage the
>>>>>> iterators issue
>>>>>> 
>>>>>> sorry for the brevity but tonight I am getting crazy with at least
3
>>>>>> other projects :D
>>>>>> 
>>>>>> -Simo
>>>>>> 
>>>>>> http://people.apache.org/~simonetripodi/
>>>>>> http://simonetripodi.livejournal.com/
>>>>>> http://twitter.com/simonetripodi
>>>>>> http://www.99soft.org/
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On Sat, Mar 3, 2012 at 1:16 AM, Marco Speranza <marcosperanza@apache.org>
wrote:
>>>>>>> I think that we have to use the same patter of java Collections:
a wrapper of Graph/MutableGraph that use a synchronize block.
>>>>>>> 
>>>>>>> WDYT?
>>>>>>> 
>>>>>>> --
>>>>>>> Marco Speranza <marcosperanza@apache.org>
>>>>>>> Google Code: http://code.google.com/u/marco.speranza79/
>>>>>>> 
>>>>>>> Il giorno 03/mar/2012, alle ore 01:10, Simone Tripodi ha scritto:
>>>>>>> 
>>>>>>>> OK now sounds better, thanks - how would you intend to fix
it?
>>>>>>>> TIA,
>>>>>>>> -Simo
>>>>>>>> 
>>>>>>>> http://people.apache.org/~simonetripodi/
>>>>>>>> http://simonetripodi.livejournal.com/
>>>>>>>> http://twitter.com/simonetripodi
>>>>>>>> http://www.99soft.org/
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On Sat, Mar 3, 2012 at 1:01 AM, Marco Speranza <marcosperanza@apache.org>
wrote:
>>>>>>>>>>> 
>>>>>>>>>>> furthermore there is another problem: with handler
is not possible to use correctly synchronization block like this
>>>>>>>>>>> 
>>>>>>>>>>> ====
>>>>>>>>>>> Graph g = CommonsGraph.synchronize(g_);
>>>>>>>>>>>     ...
>>>>>>>>>>>  synchronized(g) {
>>>>>>>>>>>       for ( BaseLabeledVertex v2 : g.getVertices()
)
>>>>>>>>>>>       {
>>>>>>>>>>>           // do somethings
>>>>>>>>>>>       }
>>>>>>>>>>>  }
>>>>>>>>>>> ====
>>>>>>>>>> 
>>>>>>>>>> sorry I don't understand what you meant/intend to
do
>>>>>>>>> 
>>>>>>>>> I'm trying to explain better. the method getVertices
return an Iterator. In a multi-thread environment you have to ensure that during the 'for'
execution the [graph] data structure remains  consistent.
>>>>>>>>> Also for java collection is the same [http://docs.oracle.com/javase/6/docs/api/java/util/Collections.html#synchronizedCollection(java.util.Collection)]
>>>>>>>>> 
>>>>>>>>> So if we use a proxy/handler there is no way to "export"
the mutex/lock variable used into handler class.
>>>>>>>>> 
>>>>>>>>> In  our CommonsGraph the variable this.lock is private
and is non possible to export out of the method, so the user can not utilize the lock to synchronize
his block.
>>>>>>>>> 
>>>>>>>>> ===
>>>>>>>>>        public Object invoke( Object proxy, Method method,
Object[] args )
>>>>>>>>>            throws Throwable
>>>>>>>>>        {
>>>>>>>>>            if ( synchronizedMethods.contains( method
) )
>>>>>>>>>            {
>>>>>>>>>                try
>>>>>>>>>                {
>>>>>>>>>                    synchronized ( this.lock )
>>>>>>>>>                    {
>>>>>>>>>                        return method.invoke( synchronizedMethods,
args );
>>>>>>>>>                    }
>>>>>>>>>                }
>>>>>>>>>                catch ( InvocationTargetException e )
>>>>>>>>>                {
>>>>>>>>>                    throw e.getTargetException();
>>>>>>>>>                }
>>>>>>>>>            }
>>>>>>>>>            return method.invoke( synchronizedMethods,
args );
>>>>>>>>>        }
>>>>>>>>> ===
>>>>>>>>> 
>>>>>>>>> ciao
>>>>>>>>> 
>>>>>>>>> --
>>>>>>>>> Marco Speranza <marcosperanza@apache.org>
>>>>>>>>> Google Code: http://code.google.com/u/marco.speranza79/
>>>>>>>>> 
>>>>>>>>> Il giorno 03/mar/2012, alle ore 00:45, Simone Tripodi
ha scritto:
>>>>>>>>> 
>>>>>>>>>>> furthermore there is another problem: with handler
is not possible to use correctly synchronization block like this
>>>>>>>>>>> 
>>>>>>>>>>> ====
>>>>>>>>>>> Graph g = CommonsGraph.synchronize(g_);
>>>>>>>>>>>     ...
>>>>>>>>>>>  synchronized(g) {
>>>>>>>>>>>       for ( BaseLabeledVertex v2 : g.getVertices()
)
>>>>>>>>>>>       {
>>>>>>>>>>>           // do somethings
>>>>>>>>>>>       }
>>>>>>>>>>>  }
>>>>>>>>>>> ====
>>>>>>>>>> 
>>>>>>>>>> sorry I don't understand what you meant/intend to
do
>>>>>>>>>> 
>>>>>>>>>>> I really think that a synchronized wrapper it's
the best solution.
>>>>>>>>>>> 
>>>>>>>>>>> WDYT?
>>>>>>>>>> 
>>>>>>>>>> they sucks because we have to keep them updated while
, but if there
>>>>>>>>>> are not alternatives...
>>>>>>>>>> -Simo
>>>>>>>>>> 
>>>>>>>>>> http://people.apache.org/~simonetripodi/
>>>>>>>>>> http://simonetripodi.livejournal.com/
>>>>>>>>>> http://twitter.com/simonetripodi
>>>>>>>>>> http://www.99soft.org/
>>>>>>>>>> 
>>>>>>>>>> ---------------------------------------------------------------------
>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> ---------------------------------------------------------------------
>>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> ---------------------------------------------------------------------
>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>> 
>>>>>> 
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>> 
>>>>> 
>>>>> 
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>> 
>>> 
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>> 
>> 
>> 
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message