[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
[aspectj-users] perthis() and cflow()

I'm seeing something similar. When a method of object A with a read
lock invokes a method on object B with a write lock, I get a failure,
since the advice incorrectly tries to upgrade/downgrade the read lock.

** Start synchronization
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.setCounter(int))
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
0, Read locks = 0]: acquiring write lock
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.setCounter(int))
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
1, Read locks = 0]: releasing write lock
execution(void org.apache.tapestry.internal.aspects.SynchTargetWrapper.run())
java.util.concurrent.locks.ReentrantReadWriteLock@30d82d[Write locks =
0, Read locks = 0]: acquiring read lock
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.incrementCounter())
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
0, Read locks = 0]: releasing read lock (for upgrade)
execution(void org.apache.tapestry.internal.aspects.SynchTargetWrapper.run())
java.util.concurrent.locks.ReentrantReadWriteLock@30d82d[Write locks =
0, Read locks = 1]: acquiring read lock
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.incrementCounter())
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
0, Read locks = 0]: releasing write lock
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.incrementCounter())
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
0, Read locks = 0]: releasing read lock (for upgrade)
execution(void org.apache.tapestry.internal.aspects.SynchTargetWrapper.run())
java.util.concurrent.locks.ReentrantReadWriteLock@30d82d[Write locks =
0, Read locks = 2]: acquiring read lock
execution(void org.apache.tapestry.internal.aspects.SynchTargetWrapper.run())
java.util.concurrent.locks.ReentrantReadWriteLock@30d82d[Write locks =
0, Read locks = 2]: acquiring read lock
execution(void org.apache.tapestry.internal.aspects.SynchTargetWrapper.run())
java.util.concurrent.locks.ReentrantReadWriteLock@30d82d[Write locks =
0, Read locks = 2]: acquiring read lock
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.incrementCounter())
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
0, Read locks = 0]: reacquiring read lock (for downgrade)
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.incrementCounter())
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
0, Read locks = 0]: releasing write lock
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.incrementCounter())
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
0, Read locks = 0]: releasing read lock (for upgrade)
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.incrementCounter())
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
0, Read locks = 0]: releasing read lock (for upgrade)
execution(void org.apache.tapestry.internal.aspects.SynchronizationTarget.incrementCounter())
java.util.concurrent.locks.ReentrantReadWriteLock@1acd47[Write locks =
0, Read locks = 0]: releasing read lock (for upgrade)
execution(void org.apache.tapestry.internal.aspects.SynchTargetWrapper.run())
java.util.concurrent.locks.ReentrantReadWriteLock@30d82d[Write locks =
0, Read locks = 5]: releasing read lock
execution(void org.apache.tapestry.internal.aspects.SynchTargetWrapper.run())
java.util.concurrent.locks.ReentrantReadWriteLock@30d82d[Write locks =
0, Read locks = 5]: acquiring read lock
Exception in thread "Thread-40" java.lang.IllegalMonitorStateException
	at java.util.concurrent.locks.ReentrantReadWriteLock$Sync.tryRelease(ReentrantReadWriteLock.java:259)
	at java.util.concurrent.locks.AbstractQueuedSynchronizer.release(AbstractQueuedSynchronizer.java:1102)
	at java.util.concurrent.locks.ReentrantReadWriteLock$WriteLock.unlock(ReentrantReadWriteLock.java:821)
	at org.apache.tapestry.internal.aspects.Synchronization.ajc$after$org_apache_tapestry_internal_aspects_Synchronization$5$796b5f14(Synchronization.aj:97)
	at org.apache.tapestry.internal.aspects.SynchronizationTarget.incrementCounter(SynchronizationTarget.java:25)
	at org.apache.tapestry.internal.aspects.SynchTargetWrapper.run(SynchTargetWrapper.java:23)
	at java.lang.Thread.run(Thread.java:595)




Method SynchTargetWrapper.run() invokes method
SynchronizationTarget.incrementCounter().

Here's the relevant portions of  my aspect:

   /** Before read lock methods, acquire the read lock. */
    before() : readMethods()
    {
        System.out.println(thisJoinPoint + " " + _lock + ": acquiring
read lock");
        _lock.readLock().lock();
    }

    /** After read lock methods (including thrown exceptions), release
the read lock. */
    after() : readMethods()
    {
        System.out.println(thisJoinPoint + " " + _lock + ": releasing
read lock");
        _lock.readLock().unlock();
    }

    /** Order counts. This advice must precede the standard
writeMethods advice below. */

    before() : writeMethods() && cflow(readMethods())
    {
        System.out.println(thisJoinPoint + " " + _lock + ": releasing
read lock (for upgrade)");
        _lock.readLock().unlock();
    }

    /**
     * Before write lock methods, acquire the write lock. Note that
obtaining the write lock will
     * block indefinately if the current thread has a read lock, but
we handle that as a special
     * case.
     */

    before(): writeMethods()
    {
        System.out.println(thisJoinPoint + " " + _lock + ": acquiring
write lock");
        _lock.writeLock().lock();
    }

    /** And release the write lock after the method completes
(successfully, or with an exception). */
    after() : writeMethods()
    {
        System.out.println(thisJoinPoint + " " + _lock + ": releasing
write lock");
        _lock.writeLock().unlock();
    }

    /**
     * After releasing the write lock we may re-acquire the read lock
(if the write method was
     * invoked from a read method).
     */
    after() : writeMethods() && cflow(readMethods())
    {
        System.out.println(thisJoinPoint + " " + _lock + ":
reacquiring read lock (for downgrade)");
        _lock.readLock().lock();
    }

... so, it looks to me like the cflow() is matching methods on
different classes and/or instances despite what Adrian said earlier.

BTW ... is it safe to assume that aspects will be applied in the order
they appear in the source?



On 4/25/06, iyad issa <iyadissa@xxxxxxxxx> wrote:
>
> this was my understanding but when i test this i found there is a cross over
>
> i.e
>
> For
>
> A {
>    @WriteLock void m1(){}
> }
>
> B {
>   A a = new A();
>   @ReadLock void m2(){
>     a.m1();
>    }
> }
>
> now call to a.m1() has been captured in the cflow of m2()
>
> I used ajdt version 1.4.0.20060306054947
>
> I will test this with the latest ajdt and report here the findings
>
>
> iyad
>
>
>
>
> On 25/04/06, Adrian Colyer <adrian.colyer@xxxxxxxxx> wrote:
> >
>
> This works due to the "implicit scoping" associated with aspect
> instantiation models. When you have a perthis(somepc()) aspect, then an
> aspect instance is created for each unique object bound to "this" at join
> points matched by somepc(). Once the aspect instance has been created, every
> advice declaration has an implicit condition 'and-ed' to its pointcut
> clause. The implicit condition for the different instantiation models is as
> follows:
>
> * perthis  ->  '&& this(the-this-object)'
> * pertarget -> '&& target(the-target-object)'
> * percflow -> '&& cflow(the-cflow-initiating-join-point)'
> * percflowbelow -> '&&
> cflowbelow(the-cflowbelow-initiating-join-point)'
> * pertypewithin -> '&& within(the-type)
>
> So for any given aspect instance, readLockMethods() only matches read lock
> methods on the advised instance, and writeLockMethods only matches
> writeLockMethods on the advised instance too. Therefore you won't get
> 'crossover' between advised instances whereby calls from one object to
> another cause inappropriate lock release.
>
> This is documented in the programming guide here:
> http://www.eclipse.org/aspectj/doc/released/progguide/semantics-aspects.html#aspect-instantiation
> , and also in the Eclipse AspectJ book (see eg. table 9.1 in chapter 9).
>
> Regards, Adrian.
>
>
>
> On 25/04/06, Howard Lewis Ship < hlship@xxxxxxxxx > wrote:
> > What about the case where you call from a read method in instance A to
> > a write method in instance B. That looks like it will release the read
> > lock on A, even though it shouldn't.
> >
> > On 4/25/06, iyad issa <iyadissa@xxxxxxxxx> wrote:
> > >
> > > how about change the before and the after advice for writeLock to the
> > > follwoing
> > >
> > >
> > >    before(): writeLockMethods() && !cflow(readLockMethods()){
> > >            _lock.writeLock().lock();
> > >    }
> > >
> > >
> > >    before(): writeLockMethods() && cflow(readLockMethods()){
> > >        _lock.readLock().unlock();
> > >
> > >        _lock.writeLock().lock();
> > >    }
> > >
> > >    /** And release the write lock after the method completes
> (successfully,
> > > or with an exception). */
> > >    after() : writeLockMethods() && !cflow(readLockMethods()){
> > >           _lock.writeLock().unlock();
> > >    }
> > >
> > >
> > >
> > >    after() : writeLockMethods() && cflow(readLockMethods()){
> > >         _lock.writeLock().unlock();
> > >        _lock.readLock().lock();
> > >    }
> > >
> > >
> > > this should work
> > >
> > >
> > > iyad
> > >
> > >
> > >
> > >
> > >
> > > On 24/04/06, Howard Lewis Ship < hlship@xxxxxxxxx> wrote:
> > > > I'm writing some synchronization aspects now.
> > > >
> > > > I want to annotation methods with @ReadLock or @WriteLock and have the
> > > > annotation manage the lock.
> > > >
> > > > So far, so good:
> > > >
> > > > package org.apache.tapestry.internal.aspects ;
> > > >
> > > > import java.util.concurrent.locks.ReadWriteLock;
> > > > import
> java.util.concurrent.locks.ReentrantReadWriteLock
> > > ;
> > > >
> > > > import
> org.apache.tapestry.internal.annotations.ReadLock ;
> > > > import
> > > org.apache.tapestry.internal.annotations.WriteLock;
> > > >
> > > > /**
> > > > * Associates a {@link
> > > java.util.concurrent.locks.ReadWriteLock} with
> > > > an object instance; the
> > > > * {@link ReadLock} and {@link WriteLock} annotations drive this.
> > > > Methods that have the ReadLock
> > > > * annotation witll be advised to obtain and release the read lock
> > > > around their execution. Methods
> > > > * with the WriteLock annotation will obtain and release the write
> > > > lock around their execution.
> > > > * Methods with ReadLock that call a WriteLock-ed method (within the
> > > > same instance) will release the
> > > > * read lock before invoking the WriteLock-ed method.
> > > > * <p>
> > > > * This aspect also enforces that the annotations are only applied to
> > > > instance (not static) methods.
> > > > *
> > > > * @author Howard M. Lewis Ship
> > > > */
> > > > public abstract aspect Synchronization extends AbstractClassTargetting
> > > > perthis(readLockMethods() || writeLockMethods())
> > > > {
> > > >    private final ReadWriteLock _lock = new ReentrantReadWriteLock();
> > > >
> > > >    declare error :
> > > >        targetClasses() &&
> > > >        execution(@(ReadLock || WriteLock) static * *(..)) :
> > > >            "ReadLock and WriteLock annotations may only be applied to
> > > > instance methods.";
> > > >
> > > >    declare error :
> > > >        targetClasses() &&
> > > >        execution(@ReadLock @WriteLock * *(..)) :
> > > >            "A method may be annotated with ReadLock or with WriteLock
> > > > but not both.";
> > > >
> > > >    pointcut readLockMethods() :
> > > >        targetClasses() &&
> > > >        execution(@ReadLock * *(..));
> > > >
> > > >    pointcut writeLockMethods() :
> > > >        targetClasses() &&
> > > >        execution(@WriteLock * *(..));
> > > >
> > > >    /** Before read lock methods, acquire the read lock. */
> > > >    before() : readLockMethods()
> > > >    {
> > > >        _lock.readLock().lock();
> > > >    }
> > > >
> > > >    /** After read lock methods (including thrown exceptions), release
> > > > the read lock. */
> > > >    after() : readLockMethods()
> > > >    {
> > > >        _lock.readLock().unlock();
> > > >    }
> > > >
> > > >    /**
> > > >     * Before write lock methods, acquire the write lock. Note that
> > > > obtaining the write lock will
> > > >     * block indefinately if the current thread has a read lock, but
> > > > we handle that as a special
> > > >     * case.
> > > >     */
> > > >
> > > >    before(): writeLockMethods()
> > > >    {
> > > >        _lock.writeLock().lock();
> > > >    }
> > > >
> > > >    /** And release the write lock after the method completes
> > > > (successfully, or with an exception). */
> > > >    after() : writeLockMethods()
> > > >    {
> > > >        _lock.writeLock().unlock();
> > > >    }
> > > >
> > > > }
> > > >
> > > >
> > > > Here's my new issues:
> > > >
> > > > 1. Using perthis() creates the aspect instance dynamically, but I'm
> > > > worried that it uses a synchronized block that will serialize my
> > > > methods after all the effort I've put in to make them highly parallel
> > > > (using the readwrite lock).
> > > >
> > > > 2. One very important case is not coverred:
> > > >
> > > > If a method with @ReadLock invokes a method OF THE SAME INSTANCE with
> > > > @WriteLock, then we need to release the read lock before we invoke the
> > > > write lock method, then re-obtain the read lock afterwards.  This
> > > > feels like something you would do with cflow(), but I can't figure out
> > > > how to bind to an instance, rather than a type.
> > > >
> > > > BTW, I'm finding the combintation of Aspects and Annotations to be
> > > > very powerful.
> > > > Using annotations to mark types or methods works well in combintaion
> > > > with a base aspect that defines an abstract targetClasses() pointcut.
> > > > Concrete aspects provide a specific set of classes for targetClasses()
> > > > and the annotation matching does the rest.
> > > >
> > > > Thanks in advance!
> > > > --
> > > > Howard M. Lewis Ship
> > > > Independent J2EE / Open-Source Java Consultant
> > > > Creator, Jakarta Tapestry
> > > > Creator, Jakarta HiveMind
> > > >
> > > > Professional Tapestry training, mentoring, support
> > > > and project work.  http://howardlewisship.com
> > > > _______________________________________________
> > > > aspectj-users mailing list
> > > > aspectj-users@xxxxxxxxxxx
> > > >
> https://dev.eclipse.org/mailman/listinfo/aspectj-users
> > > >
> > >
> > >
> > > _______________________________________________
> > > aspectj-users mailing list
> > > aspectj-users@xxxxxxxxxxx
> > > https://dev.eclipse.org/mailman/listinfo/aspectj-users
> > >
> > >
> > >
> >
> >
> > --
> > Howard M. Lewis Ship
> > Independent J2EE / Open-Source Java Consultant
> > Creator, Jakarta Tapestry
> > Creator, Jakarta HiveMind
> >
> > Professional Tapestry training, mentoring, support
> > and project work.  http://howardlewisship.com
> > _______________________________________________
> > aspectj-users mailing list
> > aspectj-users@xxxxxxxxxxx
> > https://dev.eclipse.org/mailman/listinfo/aspectj-users
> >
>
>
>
>
> --
> -- Adrian
> adrian.colyer@xxxxxxxxx
>  _______________________________________________
>
> aspectj-users mailing list
> aspectj-users@xxxxxxxxxxx
>  https://dev.eclipse.org/mailman/listinfo/aspectj-users
>
>
>
>
> _______________________________________________
> aspectj-users mailing list
> aspectj-users@xxxxxxxxxxx
> https://dev.eclipse.org/mailman/listinfo/aspectj-users
>
>
>


--
Howard M. Lewis Ship
Independent J2EE / Open-Source Java Consultant
Creator, Jakarta Tapestry
Creator, Jakarta HiveMind

Professional Tapestry training, mentoring, support
and project work.  http://howardlewisship.com