commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simone Tripodi <simonetrip...@apache.org>
Subject Re: [Graph] Test problems after last commit
Date Sat, 03 Mar 2012 16:24:36 GMT
Hi again Marco,

I though once again on this and maybe having a synchronized Graph
would be different than having a ConcurrentGraph, like
Collections.synchronizedMap() and ConcurrentHashMap, so my suggestion
is keeping the synchronized proxy as they are AND provide Concurrent*
version of actual graphs. I would implement these adapters in a proper
`concurrent` package. Does it make sense to you?

Feel free to fill an issue and assign to yourself if you want to work on it!

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 3:53 PM, Simone Tripodi <simonetripodi@apache.org> wrote:
> Hi Marco,
>
> yes I know it, what doesn't convince me is not the problem, but the
> solution. Have a look at Collections.* source code to see how  this
> problem is fixed in synchronizedMap.
>
> Anyway that confirms that maybe proxies are not the best to handle
> that situation - unless James has a solution (he's our proxy expert,
> just for the record) - and we have to manage read/write locks as James
> suggested - that it would be a nice addition
>
> -Simo
>
> http://people.apache.org/~simonetripodi/
> http://simonetripodi.livejournal.com/
> http://twitter.com/simonetripodi
> http://www.99soft.org/
>
>
>
> On Sat, Mar 3, 2012 at 3:29 PM, Marco Speranza <marcosperanza@apache.org> wrote:
>> 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
>>

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


Mime
View raw message