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 Sun, 04 Mar 2012 14:22:27 GMT
Ciao Simo,

just for summarize a little our discussion: currently the implementation committed works exactly
as  Collections.syncronized* methodos.
The workaround to export the lock outside is this:

===
GraphInvocationHandler<T> handler =   new GraphInvocationHandler<T>( synchronizedMethods,
checkedToBeSynchronized );
Object proxy = newProxyInstance( type.getClassLoader(), new Class<?>[] { type }, handler
);
handler.lock = proxy;
return type.cast( proxy );
===
First of all we create the handler that utilize 'lock' variable to open the synchronized blocks
and after the lock has been set whit the same proxy instance that will be returned.

> I though once again on this and maybe having a synchronized Graph
> would be different than having a ConcurrentGraph, like
> Collections.synchronizedMap() and ConcurrentHashMap,

why [graph] is different from a Map or List data structure? Graph it self is a data structure,
isn't it?


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

Is sounds good, but I didn't understand very well your idea. What does it means implement
an adapter?

Ciao ciao

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

Il giorno 03/mar/2012, alle ore 17:24, Simone Tripodi ha scritto:

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


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


Mime
View raw message