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 14:53:26 GMT
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