I checked out the code - honestly I didn't understand which are the benefit of managing the lock object in that way. Wouldn't have been enough synchronizing on `this`? It is what it does.
time to sleep, 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:55 AM, Simone Tripodi <simonetrip...@apache.org> wrote: >> + <repositories> >> + <repository> >> + <id>opensymphony-releases</id> >> + <name>Opensymphony Releases</name> >> + >> <url>https://oss.sonatype.org/content/repositories/opensymphony-releases</url> >> + </repository> >> + </repositories> > > not good to release the [graph] component, external repos not allowed > (and OpenSymphony dead http://www.opensymphony.com/) > >> + static private class GraphInvocationHandler<T> > > the `private`modifier should be put before; GraphInvocationHandler > class can be final > >> + protected Object lock; >> + >> + Set<Method> synchronizedMethods; >> + >> + T checkedToBeSynchronized; > > members can be final > >> + <dependency> >> + <groupId>net.sourceforge.groboutils</groupId> >> + <artifactId>groboutils-core</artifactId> >> + <version>5</version> > > that "violates" the original contract of not having dependencies - > maybe the commit message doesn't contain the `test` scope or it is > missing? > >> + @Test >> + public final void testMultiTh() > > name is not really expressive here. Test method is already marked as > @Test (so sounds a little redundant) but anyway the rest doesn't help > on understanding what the test methods performs > > HTH, 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:36 AM, <marcospera...@apache.org> wrote: >> Author: marcosperanza >> Date: Sat Mar 3 00:36:01 2012 >> New Revision: 1296530 >> >> URL: http://svn.apache.org/viewvc?rev=1296530&view=rev >> Log: >> [SANDBOX-400] Switch the Base graph implementation underlying data >> structures to the thread safe version >> >> Modified: >> commons/sandbox/graph/trunk/pom.xml >> >> commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java >> >> commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java >> >> Modified: commons/sandbox/graph/trunk/pom.xml >> URL: >> http://svn.apache.org/viewvc/commons/sandbox/graph/trunk/pom.xml?rev=1296530&r1=1296529&r2=1296530&view=diff >> ============================================================================== >> --- commons/sandbox/graph/trunk/pom.xml (original) >> +++ commons/sandbox/graph/trunk/pom.xml Sat Mar 3 00:36:01 2012 >> @@ -113,15 +113,28 @@ >> <commons.componentid>graph</commons.componentid> >> <commons.jira.componentid>12314503</commons.jira.componentid> >> </properties> >> - >> + > >> + >> <dependencies> >> <dependency> >> - <groupId>junit</groupId> >> - <artifactId>junit</artifactId> >> - <version>4.10</version> >> - <scope>test</scope> >> + <groupId>junit</groupId> >> + <artifactId>junit</artifactId> >> + <version>4.10</version> >> + <scope>test</scope> >> + </dependency> >> + <dependency> >> + <groupId>net.sourceforge.groboutils</groupId> >> + <artifactId>groboutils-core</artifactId> >> + <version>5</version> >> </dependency> >> - </dependencies> >> +</dependencies> >> >> <build> >> <resources> >> >> Modified: >> commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java >> URL: >> http://svn.apache.org/viewvc/commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java?rev=1296530&r1=1296529&r2=1296530&view=diff >> ============================================================================== >> --- >> commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java >> (original) >> +++ >> commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java >> Sat Mar 3 00:36:01 2012 >> @@ -23,6 +23,7 @@ import static java.lang.reflect.Proxy.ne >> import static org.apache.commons.graph.utils.Assertions.checkNotNull; >> >> import java.lang.reflect.InvocationHandler; >> +import java.lang.reflect.InvocationTargetException; >> import java.lang.reflect.Method; >> import java.util.HashSet; >> import java.util.Set; >> @@ -291,27 +292,49 @@ public final class CommonsGraph<V extend >> { >> synchronizedMethods.add( method ); >> } >> + GraphInvocationHandler<T> handler = >> + new GraphInvocationHandler<T>( synchronizedMethods, >> checkedToBeSynchronized ); >> + Object proxy = newProxyInstance( type.getClassLoader(), new >> Class<?>[] { type }, handler ); >> + handler.lock = proxy; >> + return type.cast( proxy ); >> + } >> + >> + >> + static private class GraphInvocationHandler<T> >> + implements InvocationHandler >> + { >> + protected Object lock; >> + >> + Set<Method> synchronizedMethods; >> + >> + T checkedToBeSynchronized; >> >> - return type.cast( newProxyInstance( type.getClassLoader(), new >> Class<?>[] { type }, >> - new InvocationHandler() >> - { >> - >> - private final Object lock = new Object(); >> - >> - public Object invoke( Object proxy, Method >> method, Object[] args ) >> - throws Throwable >> - { >> - if ( synchronizedMethods.contains( method >> ) ) >> - { >> - synchronized ( this.lock ) >> - { >> - return method.invoke( >> checkedToBeSynchronized, args ); >> - } >> - } >> - return method.invoke( >> checkedToBeSynchronized, args ); >> - } >> + public GraphInvocationHandler( Set<Method> synchronizedMethods, T >> checkedToBeSynchronized ) >> + { >> + this.synchronizedMethods = synchronizedMethods; >> + this.checkedToBeSynchronized = checkedToBeSynchronized; >> + } >> + >> + public Object invoke( Object proxy, Method method, Object[] args ) >> + throws Throwable >> + { >> + if ( synchronizedMethods.contains( method ) ) >> + { >> + synchronized ( this.lock ) >> + { >> + try >> + { >> + return method.invoke( checkedToBeSynchronized, args >> ); >> + } >> + catch ( InvocationTargetException e ) >> + { >> + throw e.getTargetException(); >> + } >> + } >> + } >> + return method.invoke( checkedToBeSynchronized, args ); >> + } >> >> - } ) ); >> } >> >> /** >> >> Modified: >> commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java >> URL: >> http://svn.apache.org/viewvc/commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java?rev=1296530&r1=1296529&r2=1296530&view=diff >> ============================================================================== >> --- >> commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java >> (original) >> +++ >> commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java >> Sat Mar 3 00:36:01 2012 >> @@ -30,7 +30,12 @@ import static org.junit.Assert.fail; >> import java.util.ArrayList; >> import java.util.List; >> >> +import net.sourceforge.groboutils.junit.v1.MultiThreadedTestRunner; >> +import net.sourceforge.groboutils.junit.v1.TestRunnable; >> + >> +import org.apache.commons.graph.CommonsGraph; >> import org.apache.commons.graph.GraphException; >> +import org.apache.commons.graph.MutableGraph; >> import org.junit.Test; >> >> /** >> @@ -367,4 +372,111 @@ public class BaseMutableGraphTestCase >> g.removeEdge( e ); >> >> } >> + >> + /** >> + * Test Graph model ina multi-thread enviroment. >> + */ >> + @Test >> + public final void testMultiTh() >> + throws Throwable >> + { >> + final MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g = >> + (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) >> CommonsGraph.synchronize( (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) >> new UndirectedMutableGraph<BaseLabeledVertex, BaseLabeledEdge>() ); >> + >> + TestRunnable tr1, tr2, tr3; >> + tr1 = new GraphInsert( g, 0, 10 ); >> + tr2 = new GraphInsert( g, 10, 20 ); >> + tr3 = new GraphInsert( g, 20, 30 ); >> + >> + TestRunnable[] trs = { tr1, tr2, tr3 }; >> + MultiThreadedTestRunner mttr = new MultiThreadedTestRunner( trs ); >> + >> + // kickstarts the MTTR & fires off threads >> + mttr.runTestRunnables(); >> + >> + assertEquals( 30, g.getOrder() ); >> + >> + // test the # of edges = n (n-1)/2 >> + assertEquals( ( 30 * ( 30 - 1 ) / 2 ), g.getSize() ); >> + } >> + >> + /** >> + * Test Graph model in a multi-thread enviroment. >> + */ >> + @Test >> + public final void testDirectedMultiTh() >> + throws Throwable >> + { >> + final MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g = >> + (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) >> CommonsGraph.synchronize( (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) >> new DirectedMutableGraph<BaseLabeledVertex, BaseLabeledEdge>() ); >> + >> + TestRunnable tr1, tr2, tr3; >> + tr1 = new GraphInsert( g, 0, 10 ); >> + tr2 = new GraphInsert( g, 10, 20 ); >> + tr3 = new GraphInsert( g, 20, 30 ); >> + >> + TestRunnable[] trs = { tr1, tr2, tr3 }; >> + MultiThreadedTestRunner mttr = new MultiThreadedTestRunner( trs ); >> + >> + // kickstarts the MTTR & fires off threads >> + mttr.runTestRunnables(); >> + >> + assertEquals( 30, g.getOrder() ); >> + >> + // test the # of edges = n (n-1) >> + assertEquals( ( 30 * ( 30 - 1 ) ), g.getSize() ); >> + } >> + >> + private class GraphInsert >> + extends TestRunnable >> + { >> + >> + private MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g; >> + >> + private int start; >> + >> + private int end; >> + >> + private GraphInsert( MutableGraph<BaseLabeledVertex, >> BaseLabeledEdge> g, int start, int end ) >> + { >> + this.g = g; >> + this.start = start; >> + this.end = end; >> + } >> + >> + public void runTest() >> + throws Throwable >> + { >> + >> + // building a complete Graph >> + for ( int i = start; i < end; i++ ) >> + { >> + BaseLabeledVertex v = new BaseLabeledVertex( valueOf( i ) ); >> + g.addVertex( v ); >> + } >> + synchronized ( g ) >> + { >> + for ( BaseLabeledVertex v1 : g.getVertices() ) >> + { >> + for ( BaseLabeledVertex v2 : g.getVertices() ) >> + { >> + if ( !v1.equals( v2 ) ) >> + { >> + try >> + { >> + BaseLabeledEdge e = new BaseLabeledEdge( v1 >> + " -> " + v2 ); >> + g.addEdge( v1, e, v2 ); >> + } >> + catch ( GraphException e ) >> + { >> + // ignore >> + } >> + } >> + } >> + } >> + >> + } >> + } >> + } >> + >> } >> >>
