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 <simonetrip...@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 <marcospera...@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 <marcospera...@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 <marcospera...@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 <marcospera...@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 <simonetrip...@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 >>>>>> <marcospera...@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 <marcospera...@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 >>>>>>>> <marcospera...@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 <marcospera...@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 >>>>>>>>>> <marcospera...@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 <marcospera...@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-unsubscr...@commons.apache.org >>>>>>>>>>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> --------------------------------------------------------------------- >>>>>>>>>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>>>>>>>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> --------------------------------------------------------------------- >>>>>>>>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>>>>>>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> --------------------------------------------------------------------- >>>>>>>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>>>>>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>>>>>>> >>>>>>>> >>>>>>>> --------------------------------------------------------------------- >>>>>>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>>>>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>>>>>> >>>>>>> >>>>>>> >>>>>>> --------------------------------------------------------------------- >>>>>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>>>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>>>>> >>>>> >>>>> --------------------------------------------------------------------- >>>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>>> >>>> >>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>> For additional commands, e-mail: dev-h...@commons.apache.org >>> >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
