ReentrantReadWriteLock with AspectJ pointcut for every initialized type of MyStructure

I am struggling to create a ReentrantReadWriteLock with AspectJ for every single object that is constructed and is a type of Mystructure. Here is my source code.

The aspect class

import org.aspectj.lang.JoinPoint;

import org.aspectj.lang.annotation.After;

import org.aspectj.lang.annotation.Aspect;

import org.aspectj.lang.annotation.Before;

import org.aspectj.lang.annotation.Pointcut;



import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



@Aspect

public class LocksAspect {

    private ReentrantReadWriteLock rwLock;

    private Lock acquireReadLock;

    private Lock acquireWriteLock;



    @Before("!within(LocksAspect)&&execution(*.new(..))")

    public void LookupBefores() {

        rwLock = new ReentrantReadWriteLock();

        acquireReadLock = rwLock.readLock();

        acquireWriteLock = rwLock.writeLock();

    }



    @Pointcut("call(void MyStructure.Insert(String))")

    public void InsertPointcut() {

    }



    @Pointcut("call(void MyStructure.Read(int))")

    public void ReadPointcut() {

    }



    @Before("InsertPointcut()")

    public void InsertPointcutBefore(JoinPoint pointcut) throws InterruptedException {

        acquireWriteLock.lock();

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + "  is entering in critical Section {} ");

        Thread.sleep(10000);

    }





    @After("InsertPointcut()")

    public void InsertPointcutAfter(JoinPoint pointcut) {

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + " received notification and is exiting critical Section {} ");

        acquireWriteLock.unlock();

    }



    @Before("ReadPointcut()")

    public void ReadPointcutBefore(JoinPoint pointcut) throws InterruptedException {

        acquireReadLock.lock();

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + "  is entering in critical Section {} ");

        Thread.sleep(1000);

    }





    @After("ReadPointcut()")

    public void ReadPointcutAfter(JoinPoint pointcut) {

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + " received notification and is exiting critical Section {} ");

        acquireReadLock.unlock();

    }

}

The Thread writer class.(The Reader thread class is not important because my problem is different so i omitted it)

public class Writer extends Thread{

   private MyStructure myStructure;

    public Writer(MyStructure myStructure) {

        this.myStructure=myStructure;

    }



    @Override

    public void run() {

        this.myStructure.Insert("example");

    }

}

My structure class

import java.util.ArrayList;



public class MyStructure {

    ArrayList<String> examplelist;



    public MyStructure() {

        examplelist = new ArrayList<String>();

    }



    public void Insert(String value) {

        examplelist.add(value);

    }



    public void Read(int pos) {

        examplelist.get(pos);

    }

}

The main

MyStructure structure = new MyStructure();

        MyStructure structure1 = new MyStructure();

        new Thread(new Writer(structure), "Thread1").start();

        new Thread(new Writer(structure1), "Thread2").start();

The output

Thread2  is entering in critical Section {} 

Thread2 received notification and is exiting critical Section {} 

Thread1  is entering in critical Section {} //Thread1 will wait for Thread2 to release the lock in critical section   which is wrong

Thread1 received notification and is exiting critical Section {}

Now my problem is How I will get a new ReentrantReadWriteLock for each object of Mystructure that created. For example, if we run the above example both Thread1 and Thread2 must be able to access the critical section because they have different references of the object, but this should not have happened. My problem is that the Thread2 will block and wait for Thread1 to finish which is wrong. How can I bypass this problem of construction with Aspect4j?

edited Dec 7 '18 at 23:23

halfer

14.4k758109

asked Nov 12 '18 at 22:23

Mixalis Navridis

428

add a comment |

I am struggling to create a ReentrantReadWriteLock with AspectJ for every single object that is constructed and is a type of Mystructure. Here is my source code.

The aspect class

import org.aspectj.lang.JoinPoint;

import org.aspectj.lang.annotation.After;

import org.aspectj.lang.annotation.Aspect;

import org.aspectj.lang.annotation.Before;

import org.aspectj.lang.annotation.Pointcut;



import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



@Aspect

public class LocksAspect {

    private ReentrantReadWriteLock rwLock;

    private Lock acquireReadLock;

    private Lock acquireWriteLock;



    @Before("!within(LocksAspect)&&execution(*.new(..))")

    public void LookupBefores() {

        rwLock = new ReentrantReadWriteLock();

        acquireReadLock = rwLock.readLock();

        acquireWriteLock = rwLock.writeLock();

    }



    @Pointcut("call(void MyStructure.Insert(String))")

    public void InsertPointcut() {

    }



    @Pointcut("call(void MyStructure.Read(int))")

    public void ReadPointcut() {

    }



    @Before("InsertPointcut()")

    public void InsertPointcutBefore(JoinPoint pointcut) throws InterruptedException {

        acquireWriteLock.lock();

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + "  is entering in critical Section {} ");

        Thread.sleep(10000);

    }





    @After("InsertPointcut()")

    public void InsertPointcutAfter(JoinPoint pointcut) {

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + " received notification and is exiting critical Section {} ");

        acquireWriteLock.unlock();

    }



    @Before("ReadPointcut()")

    public void ReadPointcutBefore(JoinPoint pointcut) throws InterruptedException {

        acquireReadLock.lock();

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + "  is entering in critical Section {} ");

        Thread.sleep(1000);

    }





    @After("ReadPointcut()")

    public void ReadPointcutAfter(JoinPoint pointcut) {

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + " received notification and is exiting critical Section {} ");

        acquireReadLock.unlock();

    }

}

The Thread writer class.(The Reader thread class is not important because my problem is different so i omitted it)

public class Writer extends Thread{

   private MyStructure myStructure;

    public Writer(MyStructure myStructure) {

        this.myStructure=myStructure;

    }



    @Override

    public void run() {

        this.myStructure.Insert("example");

    }

}

My structure class

import java.util.ArrayList;



public class MyStructure {

    ArrayList<String> examplelist;



    public MyStructure() {

        examplelist = new ArrayList<String>();

    }



    public void Insert(String value) {

        examplelist.add(value);

    }



    public void Read(int pos) {

        examplelist.get(pos);

    }

}

The main

MyStructure structure = new MyStructure();

        MyStructure structure1 = new MyStructure();

        new Thread(new Writer(structure), "Thread1").start();

        new Thread(new Writer(structure1), "Thread2").start();

The output

Thread2  is entering in critical Section {} 

Thread2 received notification and is exiting critical Section {} 

Thread1  is entering in critical Section {} //Thread1 will wait for Thread2 to release the lock in critical section   which is wrong

Thread1 received notification and is exiting critical Section {}

edited Dec 7 '18 at 23:23

halfer

14.4k758109

asked Nov 12 '18 at 22:23

Mixalis Navridis

428

add a comment |

I am struggling to create a ReentrantReadWriteLock with AspectJ for every single object that is constructed and is a type of Mystructure. Here is my source code.

The aspect class

import org.aspectj.lang.JoinPoint;

import org.aspectj.lang.annotation.After;

import org.aspectj.lang.annotation.Aspect;

import org.aspectj.lang.annotation.Before;

import org.aspectj.lang.annotation.Pointcut;



import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



@Aspect

public class LocksAspect {

    private ReentrantReadWriteLock rwLock;

    private Lock acquireReadLock;

    private Lock acquireWriteLock;



    @Before("!within(LocksAspect)&&execution(*.new(..))")

    public void LookupBefores() {

        rwLock = new ReentrantReadWriteLock();

        acquireReadLock = rwLock.readLock();

        acquireWriteLock = rwLock.writeLock();

    }



    @Pointcut("call(void MyStructure.Insert(String))")

    public void InsertPointcut() {

    }



    @Pointcut("call(void MyStructure.Read(int))")

    public void ReadPointcut() {

    }



    @Before("InsertPointcut()")

    public void InsertPointcutBefore(JoinPoint pointcut) throws InterruptedException {

        acquireWriteLock.lock();

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + "  is entering in critical Section {} ");

        Thread.sleep(10000);

    }





    @After("InsertPointcut()")

    public void InsertPointcutAfter(JoinPoint pointcut) {

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + " received notification and is exiting critical Section {} ");

        acquireWriteLock.unlock();

    }



    @Before("ReadPointcut()")

    public void ReadPointcutBefore(JoinPoint pointcut) throws InterruptedException {

        acquireReadLock.lock();

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + "  is entering in critical Section {} ");

        Thread.sleep(1000);

    }





    @After("ReadPointcut()")

    public void ReadPointcutAfter(JoinPoint pointcut) {

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + " received notification and is exiting critical Section {} ");

        acquireReadLock.unlock();

    }

}

The Thread writer class.(The Reader thread class is not important because my problem is different so i omitted it)

public class Writer extends Thread{

   private MyStructure myStructure;

    public Writer(MyStructure myStructure) {

        this.myStructure=myStructure;

    }



    @Override

    public void run() {

        this.myStructure.Insert("example");

    }

}

My structure class

import java.util.ArrayList;



public class MyStructure {

    ArrayList<String> examplelist;



    public MyStructure() {

        examplelist = new ArrayList<String>();

    }



    public void Insert(String value) {

        examplelist.add(value);

    }



    public void Read(int pos) {

        examplelist.get(pos);

    }

}

The main

MyStructure structure = new MyStructure();

        MyStructure structure1 = new MyStructure();

        new Thread(new Writer(structure), "Thread1").start();

        new Thread(new Writer(structure1), "Thread2").start();

The output

Thread2  is entering in critical Section {} 

Thread2 received notification and is exiting critical Section {} 

Thread1  is entering in critical Section {} //Thread1 will wait for Thread2 to release the lock in critical section   which is wrong

Thread1 received notification and is exiting critical Section {}

edited Dec 7 '18 at 23:23

halfer

14.4k758109

asked Nov 12 '18 at 22:23

Mixalis Navridis

428

I am struggling to create a ReentrantReadWriteLock with AspectJ for every single object that is constructed and is a type of Mystructure. Here is my source code.

The aspect class

import org.aspectj.lang.JoinPoint;

import org.aspectj.lang.annotation.After;

import org.aspectj.lang.annotation.Aspect;

import org.aspectj.lang.annotation.Before;

import org.aspectj.lang.annotation.Pointcut;



import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



@Aspect

public class LocksAspect {

    private ReentrantReadWriteLock rwLock;

    private Lock acquireReadLock;

    private Lock acquireWriteLock;



    @Before("!within(LocksAspect)&&execution(*.new(..))")

    public void LookupBefores() {

        rwLock = new ReentrantReadWriteLock();

        acquireReadLock = rwLock.readLock();

        acquireWriteLock = rwLock.writeLock();

    }



    @Pointcut("call(void MyStructure.Insert(String))")

    public void InsertPointcut() {

    }



    @Pointcut("call(void MyStructure.Read(int))")

    public void ReadPointcut() {

    }



    @Before("InsertPointcut()")

    public void InsertPointcutBefore(JoinPoint pointcut) throws InterruptedException {

        acquireWriteLock.lock();

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + "  is entering in critical Section {} ");

        Thread.sleep(10000);

    }





    @After("InsertPointcut()")

    public void InsertPointcutAfter(JoinPoint pointcut) {

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + " received notification and is exiting critical Section {} ");

        acquireWriteLock.unlock();

    }



    @Before("ReadPointcut()")

    public void ReadPointcutBefore(JoinPoint pointcut) throws InterruptedException {

        acquireReadLock.lock();

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + "  is entering in critical Section {} ");

        Thread.sleep(1000);

    }





    @After("ReadPointcut()")

    public void ReadPointcutAfter(JoinPoint pointcut) {

        String thrdName = Thread.currentThread().getName();

        System.out.println(thrdName + " received notification and is exiting critical Section {} ");

        acquireReadLock.unlock();

    }

}

The Thread writer class.(The Reader thread class is not important because my problem is different so i omitted it)

public class Writer extends Thread{

   private MyStructure myStructure;

    public Writer(MyStructure myStructure) {

        this.myStructure=myStructure;

    }



    @Override

    public void run() {

        this.myStructure.Insert("example");

    }

}

My structure class

import java.util.ArrayList;



public class MyStructure {

    ArrayList<String> examplelist;



    public MyStructure() {

        examplelist = new ArrayList<String>();

    }



    public void Insert(String value) {

        examplelist.add(value);

    }



    public void Read(int pos) {

        examplelist.get(pos);

    }

}

The main

MyStructure structure = new MyStructure();

        MyStructure structure1 = new MyStructure();

        new Thread(new Writer(structure), "Thread1").start();

        new Thread(new Writer(structure1), "Thread2").start();

The output

Thread2  is entering in critical Section {} 

Thread2 received notification and is exiting critical Section {} 

Thread1  is entering in critical Section {} //Thread1 will wait for Thread2 to release the lock in critical section   which is wrong

Thread1 received notification and is exiting critical Section {}

java concurrency aop aspectj pointcut

edited Dec 7 '18 at 23:23

halfer

14.4k758109

asked Nov 12 '18 at 22:23

Mixalis Navridis

428

edited Dec 7 '18 at 23:23

halfer

14.4k758109

asked Nov 12 '18 at 22:23

Mixalis Navridis

428

edited Dec 7 '18 at 23:23

halfer

14.4k758109

edited Dec 7 '18 at 23:23

halfer

14.4k758109

edited Dec 7 '18 at 23:23

halfer

14.4k758109

asked Nov 12 '18 at 22:23

Mixalis Navridis

428

asked Nov 12 '18 at 22:23

Mixalis Navridis

428

asked Nov 12 '18 at 22:23

Mixalis Navridis

428

add a comment |

1 Answer
1

active

oldest

votes

The key to your problem's solution is that you need one set of locks per MyStructure instance. Your aspect is a singleton, though. So either you need to use another aspect instantiation scheme (which is what I will use in my answer) or do manual bookkeeping within the singleton aspect by keeping a set of locks and add a new element into that set whenever a MyStructure object is created.

In order to better understand my answer, please refer to the AspectJ manual for information about aspect instantiation.

Before we start, a few comments concerning your code and why I have changed it a bit:

Your Writer is already a Thread subclass, no need to wrap it into another thread instance. (I know you probably just did it for being able to name the thread, but that could have been achieved by adding a constructor in your class taking the name argument and passing it through to the super class constructor.)

You should not call a variable of type JoinPoint pointcut because a joinpoint is not a pointcut AOP-wise.

I factored out the logging into its own helper method and improved it a bit so we can see more clearly what happens when.

I decided to replace each pair of before and after advice by an around advice. This is optional, of course, but I prefer to see the control flow in one place in this case. BTW, be careful to change the around advice's return type to Object and actually return something if you want to target non-void methods. Here it is not necessary because in both cases we have void methods.

I also decided to inline the pointcuts, which is also optional, but makes the sample code somewhat more concise for demonstration purposes here.

I added a Reader class and use it in order to show the difference between reentrant read vs write locks.

I also took care of making MyStructure instances nameable and printable in order to identify the target objects more easily in the log.

I randomised the execution order of reader/writer threads so as to mix them in a more real-world fashion. In order to avoid exceptions polluting the log when reading from a newly created MyStructure before writing into, I made sure that MyStructure gets a default element right in the constructor. I did not want to catch exceptions here to in order to keep the sample code simple.

I put the aspect in another package than the application code in order to demonstrate than generally you need to use fully qualified class names when using annotation-style AspectJ (in native syntax imports would be enough).

Now what is the solution? Basically just this, because the changes mentioned above just make the code better or the test program closer to real-life situations:

@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect { // (...)

This creates one aspect instance per MyStructure object. This is also why we can assign the values of readWriteLock, readLock and writeLock directly instead of using a special pointcut + advice pair like in your singleton aspect.

Here is the full, refactored sample code:

Application code + driver application:

package de.scrum_master.app;



import java.util.ArrayList;

import java.util.List;



public class MyStructure {

  private String name;

  private List<String> myList;



  public MyStructure(String name) {

    this.name = name;

    myList = new ArrayList<String>();

    myList.add("dummy element to permit reading");

  }



  public void insert(String value) {

    myList.add(value);

  }



  public void read(int pos) {

    myList.get(pos);

  }



  @Override

  public String toString() {

    return "MyStructure[" + name + "]";

  }

}

package de.scrum_master.app;



public class Writer extends Thread {

  private MyStructure myStructure;



  public Writer(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.insert("example");

  }

}

package de.scrum_master.app;



public class Reader extends Thread {

  private MyStructure myStructure;



  public Reader(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.read(0);

  }

}

package de.scrum_master.app;



import java.util.Arrays;

import java.util.Collections;

import java.util.List;



public class Application {

  public static void main(String args) {

    MyStructure structureA = new MyStructure("One");

    MyStructure structureB = new MyStructure("Two");

    List<Thread> threads = Arrays.asList(

      new Writer(structureA), new Writer(structureB), new Writer(structureA), new Writer(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB)

    );

    Collections.shuffle(threads);

    for (Thread thread : threads)

      thread.start();

  }

}

Aspect:

package de.scrum_master.aspect;



import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



import org.aspectj.lang.ProceedingJoinPoint;

import org.aspectj.lang.annotation.Around;

import org.aspectj.lang.annotation.Aspect;



import de.scrum_master.app.MyStructure;



@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect {

  private static final long startTime = System.currentTimeMillis();



  private ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock();

  private Lock readLock = readWriteLock.readLock();

  private Lock writeLock = readWriteLock.writeLock();



  @Around("target(myStructure) && execution(void insert(String))")

  public void InsertPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    writeLock.lock();

    log("entering write section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting write section", myStructure);

      writeLock.unlock();

    }

  }



  @Around("target(myStructure) && execution(void read(int))")

  public void ReadPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    readLock.lock();

    log("entering read section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting read section", myStructure);

      readLock.unlock();

    }

  }



  private static void log(String message, Object targetObject)  {

    System.out.printf(

      "%8d ms | %-25s | %-17s | %s%n",

      System.currentTimeMillis() - startTime,

      Thread.currentThread(),

      targetObject,

      message

    );

  }

}

Sample log output:

       4 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | entering write section

       4 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | entering read section

    1019 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | exiting write section

    1020 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | exiting read section

    1025 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | entering write section

    2023 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | exiting read section

    2024 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | exiting read section

    2025 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | entering write section

    2026 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | exiting write section

    2026 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | entering write section

    3026 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | exiting write section

    3026 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | entering read section

    3026 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | entering read section

    3028 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | exiting write section

    4028 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | exiting read section

    4029 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | exiting read section

edited Nov 23 '18 at 11:42

answered Nov 22 '18 at 8:19

kriegaex

30.8k363100

Minor, unrelated to aspects comment. The idiom (I think) is to acquire a lock, and then enter the try/finally, not try, then acquire, then finally. The idea being that if an exception (esp. InterruptedException) is thrown while acquiring, you do not want to enter the finally clause and release a lock you never successfully acquired in the first place. Please correct me if I'm mistaken.
– GPI
Nov 22 '18 at 8:47

I know that. The try-finally is for calling proceed as well or whatever else could go wrong and I did not want to nest it. You cannot look at code inside an aspect from a non-aspect perspective only. BTW, proceed() can throw Throwable, but whatever happens, the finally block should execute.
– kriegaex
Nov 22 '18 at 14:10

"whatever happens, the finally block should execute". This is where I disagree. If the lock was successfully acquired then the finally block should execute no matter what. But if the lock was not acquired, then the finally block must not be executed (that would release a lock that was never acquired. If your lock is semaphore based, it would release permits that never meant to exist). By placing lock.lock() inside the try, you open up the possibility of failing to acquire and releasing nonetheless, which will at best fail, and at worse trigger unspecified behavior.
– GPI
Nov 23 '18 at 11:25

Yesterday I looked at too quickly, my bad. I am stupid and ugly and you are right. I updated my code accordingly, moving the lock operations to before the respective 'try' blocks. Thanks for your valuable comments! :-)
– kriegaex
Nov 23 '18 at 11:44

Not an issue. I had a feeling we were not understanding each other. Very nice answer, by the way, +1.
– GPI
Nov 23 '18 at 15:51

add a comment |

Your Answer

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "1"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: true,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: 10,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});

}
});

draft saved

draft discarded

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53270967%2freentrantreadwritelock-with-aspectj-pointcut-for-every-initialized-type-of-mystr%23new-answer', 'question_page');
}
);

Post as a guest

Name

Required, but never shown

1 Answer
1

active

oldest

votes

1 Answer
1

active

oldest

votes

In order to better understand my answer, please refer to the AspectJ manual for information about aspect instantiation.

Before we start, a few comments concerning your code and why I have changed it a bit:

Your Writer is already a Thread subclass, no need to wrap it into another thread instance. (I know you probably just did it for being able to name the thread, but that could have been achieved by adding a constructor in your class taking the name argument and passing it through to the super class constructor.)

You should not call a variable of type JoinPoint pointcut because a joinpoint is not a pointcut AOP-wise.

I factored out the logging into its own helper method and improved it a bit so we can see more clearly what happens when.

I decided to replace each pair of before and after advice by an around advice. This is optional, of course, but I prefer to see the control flow in one place in this case. BTW, be careful to change the around advice's return type to Object and actually return something if you want to target non-void methods. Here it is not necessary because in both cases we have void methods.

I also decided to inline the pointcuts, which is also optional, but makes the sample code somewhat more concise for demonstration purposes here.

I added a Reader class and use it in order to show the difference between reentrant read vs write locks.

I also took care of making MyStructure instances nameable and printable in order to identify the target objects more easily in the log.

I randomised the execution order of reader/writer threads so as to mix them in a more real-world fashion. In order to avoid exceptions polluting the log when reading from a newly created MyStructure before writing into, I made sure that MyStructure gets a default element right in the constructor. I did not want to catch exceptions here to in order to keep the sample code simple.

I put the aspect in another package than the application code in order to demonstrate than generally you need to use fully qualified class names when using annotation-style AspectJ (in native syntax imports would be enough).

Now what is the solution? Basically just this, because the changes mentioned above just make the code better or the test program closer to real-life situations:

@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect { // (...)

Here is the full, refactored sample code:

Application code + driver application:

package de.scrum_master.app;



import java.util.ArrayList;

import java.util.List;



public class MyStructure {

  private String name;

  private List<String> myList;



  public MyStructure(String name) {

    this.name = name;

    myList = new ArrayList<String>();

    myList.add("dummy element to permit reading");

  }



  public void insert(String value) {

    myList.add(value);

  }



  public void read(int pos) {

    myList.get(pos);

  }



  @Override

  public String toString() {

    return "MyStructure[" + name + "]";

  }

}

package de.scrum_master.app;



public class Writer extends Thread {

  private MyStructure myStructure;



  public Writer(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.insert("example");

  }

}

package de.scrum_master.app;



public class Reader extends Thread {

  private MyStructure myStructure;



  public Reader(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.read(0);

  }

}

package de.scrum_master.app;



import java.util.Arrays;

import java.util.Collections;

import java.util.List;



public class Application {

  public static void main(String args) {

    MyStructure structureA = new MyStructure("One");

    MyStructure structureB = new MyStructure("Two");

    List<Thread> threads = Arrays.asList(

      new Writer(structureA), new Writer(structureB), new Writer(structureA), new Writer(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB)

    );

    Collections.shuffle(threads);

    for (Thread thread : threads)

      thread.start();

  }

}

Aspect:

package de.scrum_master.aspect;



import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



import org.aspectj.lang.ProceedingJoinPoint;

import org.aspectj.lang.annotation.Around;

import org.aspectj.lang.annotation.Aspect;



import de.scrum_master.app.MyStructure;



@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect {

  private static final long startTime = System.currentTimeMillis();



  private ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock();

  private Lock readLock = readWriteLock.readLock();

  private Lock writeLock = readWriteLock.writeLock();



  @Around("target(myStructure) && execution(void insert(String))")

  public void InsertPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    writeLock.lock();

    log("entering write section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting write section", myStructure);

      writeLock.unlock();

    }

  }



  @Around("target(myStructure) && execution(void read(int))")

  public void ReadPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    readLock.lock();

    log("entering read section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting read section", myStructure);

      readLock.unlock();

    }

  }



  private static void log(String message, Object targetObject)  {

    System.out.printf(

      "%8d ms | %-25s | %-17s | %s%n",

      System.currentTimeMillis() - startTime,

      Thread.currentThread(),

      targetObject,

      message

    );

  }

}

Sample log output:

       4 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | entering write section

       4 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | entering read section

    1019 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | exiting write section

    1020 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | exiting read section

    1025 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | entering write section

    2023 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | exiting read section

    2024 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | exiting read section

    2025 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | entering write section

    2026 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | exiting write section

    2026 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | entering write section

    3026 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | exiting write section

    3026 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | entering read section

    3026 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | entering read section

    3028 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | exiting write section

    4028 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | exiting read section

    4029 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | exiting read section

edited Nov 23 '18 at 11:42

answered Nov 22 '18 at 8:19

kriegaex

30.8k363100

Minor, unrelated to aspects comment. The idiom (I think) is to acquire a lock, and then enter the try/finally, not try, then acquire, then finally. The idea being that if an exception (esp. InterruptedException) is thrown while acquiring, you do not want to enter the finally clause and release a lock you never successfully acquired in the first place. Please correct me if I'm mistaken.
– GPI
Nov 22 '18 at 8:47

I know that. The try-finally is for calling proceed as well or whatever else could go wrong and I did not want to nest it. You cannot look at code inside an aspect from a non-aspect perspective only. BTW, proceed() can throw Throwable, but whatever happens, the finally block should execute.
– kriegaex
Nov 22 '18 at 14:10

"whatever happens, the finally block should execute". This is where I disagree. If the lock was successfully acquired then the finally block should execute no matter what. But if the lock was not acquired, then the finally block must not be executed (that would release a lock that was never acquired. If your lock is semaphore based, it would release permits that never meant to exist). By placing lock.lock() inside the try, you open up the possibility of failing to acquire and releasing nonetheless, which will at best fail, and at worse trigger unspecified behavior.
– GPI
Nov 23 '18 at 11:25

Yesterday I looked at too quickly, my bad. I am stupid and ugly and you are right. I updated my code accordingly, moving the lock operations to before the respective 'try' blocks. Thanks for your valuable comments! :-)
– kriegaex
Nov 23 '18 at 11:44

Not an issue. I had a feeling we were not understanding each other. Very nice answer, by the way, +1.
– GPI
Nov 23 '18 at 15:51

add a comment |

In order to better understand my answer, please refer to the AspectJ manual for information about aspect instantiation.

Before we start, a few comments concerning your code and why I have changed it a bit:

Your Writer is already a Thread subclass, no need to wrap it into another thread instance. (I know you probably just did it for being able to name the thread, but that could have been achieved by adding a constructor in your class taking the name argument and passing it through to the super class constructor.)

You should not call a variable of type JoinPoint pointcut because a joinpoint is not a pointcut AOP-wise.

I factored out the logging into its own helper method and improved it a bit so we can see more clearly what happens when.

I decided to replace each pair of before and after advice by an around advice. This is optional, of course, but I prefer to see the control flow in one place in this case. BTW, be careful to change the around advice's return type to Object and actually return something if you want to target non-void methods. Here it is not necessary because in both cases we have void methods.

I also decided to inline the pointcuts, which is also optional, but makes the sample code somewhat more concise for demonstration purposes here.

I added a Reader class and use it in order to show the difference between reentrant read vs write locks.

I also took care of making MyStructure instances nameable and printable in order to identify the target objects more easily in the log.

I randomised the execution order of reader/writer threads so as to mix them in a more real-world fashion. In order to avoid exceptions polluting the log when reading from a newly created MyStructure before writing into, I made sure that MyStructure gets a default element right in the constructor. I did not want to catch exceptions here to in order to keep the sample code simple.

I put the aspect in another package than the application code in order to demonstrate than generally you need to use fully qualified class names when using annotation-style AspectJ (in native syntax imports would be enough).

Now what is the solution? Basically just this, because the changes mentioned above just make the code better or the test program closer to real-life situations:

@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect { // (...)

Here is the full, refactored sample code:

Application code + driver application:

package de.scrum_master.app;



import java.util.ArrayList;

import java.util.List;



public class MyStructure {

  private String name;

  private List<String> myList;



  public MyStructure(String name) {

    this.name = name;

    myList = new ArrayList<String>();

    myList.add("dummy element to permit reading");

  }



  public void insert(String value) {

    myList.add(value);

  }



  public void read(int pos) {

    myList.get(pos);

  }



  @Override

  public String toString() {

    return "MyStructure[" + name + "]";

  }

}

package de.scrum_master.app;



public class Writer extends Thread {

  private MyStructure myStructure;



  public Writer(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.insert("example");

  }

}

package de.scrum_master.app;



public class Reader extends Thread {

  private MyStructure myStructure;



  public Reader(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.read(0);

  }

}

package de.scrum_master.app;



import java.util.Arrays;

import java.util.Collections;

import java.util.List;



public class Application {

  public static void main(String args) {

    MyStructure structureA = new MyStructure("One");

    MyStructure structureB = new MyStructure("Two");

    List<Thread> threads = Arrays.asList(

      new Writer(structureA), new Writer(structureB), new Writer(structureA), new Writer(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB)

    );

    Collections.shuffle(threads);

    for (Thread thread : threads)

      thread.start();

  }

}

Aspect:

package de.scrum_master.aspect;



import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



import org.aspectj.lang.ProceedingJoinPoint;

import org.aspectj.lang.annotation.Around;

import org.aspectj.lang.annotation.Aspect;



import de.scrum_master.app.MyStructure;



@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect {

  private static final long startTime = System.currentTimeMillis();



  private ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock();

  private Lock readLock = readWriteLock.readLock();

  private Lock writeLock = readWriteLock.writeLock();



  @Around("target(myStructure) && execution(void insert(String))")

  public void InsertPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    writeLock.lock();

    log("entering write section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting write section", myStructure);

      writeLock.unlock();

    }

  }



  @Around("target(myStructure) && execution(void read(int))")

  public void ReadPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    readLock.lock();

    log("entering read section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting read section", myStructure);

      readLock.unlock();

    }

  }



  private static void log(String message, Object targetObject)  {

    System.out.printf(

      "%8d ms | %-25s | %-17s | %s%n",

      System.currentTimeMillis() - startTime,

      Thread.currentThread(),

      targetObject,

      message

    );

  }

}

Sample log output:

       4 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | entering write section

       4 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | entering read section

    1019 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | exiting write section

    1020 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | exiting read section

    1025 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | entering write section

    2023 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | exiting read section

    2024 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | exiting read section

    2025 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | entering write section

    2026 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | exiting write section

    2026 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | entering write section

    3026 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | exiting write section

    3026 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | entering read section

    3026 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | entering read section

    3028 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | exiting write section

    4028 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | exiting read section

    4029 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | exiting read section

edited Nov 23 '18 at 11:42

answered Nov 22 '18 at 8:19

kriegaex

30.8k363100

Minor, unrelated to aspects comment. The idiom (I think) is to acquire a lock, and then enter the try/finally, not try, then acquire, then finally. The idea being that if an exception (esp. InterruptedException) is thrown while acquiring, you do not want to enter the finally clause and release a lock you never successfully acquired in the first place. Please correct me if I'm mistaken.
– GPI
Nov 22 '18 at 8:47

I know that. The try-finally is for calling proceed as well or whatever else could go wrong and I did not want to nest it. You cannot look at code inside an aspect from a non-aspect perspective only. BTW, proceed() can throw Throwable, but whatever happens, the finally block should execute.
– kriegaex
Nov 22 '18 at 14:10

"whatever happens, the finally block should execute". This is where I disagree. If the lock was successfully acquired then the finally block should execute no matter what. But if the lock was not acquired, then the finally block must not be executed (that would release a lock that was never acquired. If your lock is semaphore based, it would release permits that never meant to exist). By placing lock.lock() inside the try, you open up the possibility of failing to acquire and releasing nonetheless, which will at best fail, and at worse trigger unspecified behavior.
– GPI
Nov 23 '18 at 11:25

Yesterday I looked at too quickly, my bad. I am stupid and ugly and you are right. I updated my code accordingly, moving the lock operations to before the respective 'try' blocks. Thanks for your valuable comments! :-)
– kriegaex
Nov 23 '18 at 11:44

Not an issue. I had a feeling we were not understanding each other. Very nice answer, by the way, +1.
– GPI
Nov 23 '18 at 15:51

add a comment |

In order to better understand my answer, please refer to the AspectJ manual for information about aspect instantiation.

Before we start, a few comments concerning your code and why I have changed it a bit:

Your Writer is already a Thread subclass, no need to wrap it into another thread instance. (I know you probably just did it for being able to name the thread, but that could have been achieved by adding a constructor in your class taking the name argument and passing it through to the super class constructor.)

You should not call a variable of type JoinPoint pointcut because a joinpoint is not a pointcut AOP-wise.

I factored out the logging into its own helper method and improved it a bit so we can see more clearly what happens when.

I decided to replace each pair of before and after advice by an around advice. This is optional, of course, but I prefer to see the control flow in one place in this case. BTW, be careful to change the around advice's return type to Object and actually return something if you want to target non-void methods. Here it is not necessary because in both cases we have void methods.

I also decided to inline the pointcuts, which is also optional, but makes the sample code somewhat more concise for demonstration purposes here.

I added a Reader class and use it in order to show the difference between reentrant read vs write locks.

I also took care of making MyStructure instances nameable and printable in order to identify the target objects more easily in the log.

I randomised the execution order of reader/writer threads so as to mix them in a more real-world fashion. In order to avoid exceptions polluting the log when reading from a newly created MyStructure before writing into, I made sure that MyStructure gets a default element right in the constructor. I did not want to catch exceptions here to in order to keep the sample code simple.

I put the aspect in another package than the application code in order to demonstrate than generally you need to use fully qualified class names when using annotation-style AspectJ (in native syntax imports would be enough).

Now what is the solution? Basically just this, because the changes mentioned above just make the code better or the test program closer to real-life situations:

@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect { // (...)

Here is the full, refactored sample code:

Application code + driver application:

package de.scrum_master.app;



import java.util.ArrayList;

import java.util.List;



public class MyStructure {

  private String name;

  private List<String> myList;



  public MyStructure(String name) {

    this.name = name;

    myList = new ArrayList<String>();

    myList.add("dummy element to permit reading");

  }



  public void insert(String value) {

    myList.add(value);

  }



  public void read(int pos) {

    myList.get(pos);

  }



  @Override

  public String toString() {

    return "MyStructure[" + name + "]";

  }

}

package de.scrum_master.app;



public class Writer extends Thread {

  private MyStructure myStructure;



  public Writer(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.insert("example");

  }

}

package de.scrum_master.app;



public class Reader extends Thread {

  private MyStructure myStructure;



  public Reader(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.read(0);

  }

}

package de.scrum_master.app;



import java.util.Arrays;

import java.util.Collections;

import java.util.List;



public class Application {

  public static void main(String args) {

    MyStructure structureA = new MyStructure("One");

    MyStructure structureB = new MyStructure("Two");

    List<Thread> threads = Arrays.asList(

      new Writer(structureA), new Writer(structureB), new Writer(structureA), new Writer(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB)

    );

    Collections.shuffle(threads);

    for (Thread thread : threads)

      thread.start();

  }

}

Aspect:

package de.scrum_master.aspect;



import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



import org.aspectj.lang.ProceedingJoinPoint;

import org.aspectj.lang.annotation.Around;

import org.aspectj.lang.annotation.Aspect;



import de.scrum_master.app.MyStructure;



@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect {

  private static final long startTime = System.currentTimeMillis();



  private ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock();

  private Lock readLock = readWriteLock.readLock();

  private Lock writeLock = readWriteLock.writeLock();



  @Around("target(myStructure) && execution(void insert(String))")

  public void InsertPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    writeLock.lock();

    log("entering write section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting write section", myStructure);

      writeLock.unlock();

    }

  }



  @Around("target(myStructure) && execution(void read(int))")

  public void ReadPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    readLock.lock();

    log("entering read section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting read section", myStructure);

      readLock.unlock();

    }

  }



  private static void log(String message, Object targetObject)  {

    System.out.printf(

      "%8d ms | %-25s | %-17s | %s%n",

      System.currentTimeMillis() - startTime,

      Thread.currentThread(),

      targetObject,

      message

    );

  }

}

Sample log output:

       4 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | entering write section

       4 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | entering read section

    1019 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | exiting write section

    1020 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | exiting read section

    1025 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | entering write section

    2023 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | exiting read section

    2024 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | exiting read section

    2025 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | entering write section

    2026 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | exiting write section

    2026 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | entering write section

    3026 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | exiting write section

    3026 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | entering read section

    3026 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | entering read section

    3028 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | exiting write section

    4028 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | exiting read section

    4029 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | exiting read section

edited Nov 23 '18 at 11:42

answered Nov 22 '18 at 8:19

kriegaex

30.8k363100

In order to better understand my answer, please refer to the AspectJ manual for information about aspect instantiation.

Before we start, a few comments concerning your code and why I have changed it a bit:

Your Writer is already a Thread subclass, no need to wrap it into another thread instance. (I know you probably just did it for being able to name the thread, but that could have been achieved by adding a constructor in your class taking the name argument and passing it through to the super class constructor.)

You should not call a variable of type JoinPoint pointcut because a joinpoint is not a pointcut AOP-wise.

I factored out the logging into its own helper method and improved it a bit so we can see more clearly what happens when.

I decided to replace each pair of before and after advice by an around advice. This is optional, of course, but I prefer to see the control flow in one place in this case. BTW, be careful to change the around advice's return type to Object and actually return something if you want to target non-void methods. Here it is not necessary because in both cases we have void methods.

I also decided to inline the pointcuts, which is also optional, but makes the sample code somewhat more concise for demonstration purposes here.

I added a Reader class and use it in order to show the difference between reentrant read vs write locks.

I also took care of making MyStructure instances nameable and printable in order to identify the target objects more easily in the log.

I randomised the execution order of reader/writer threads so as to mix them in a more real-world fashion. In order to avoid exceptions polluting the log when reading from a newly created MyStructure before writing into, I made sure that MyStructure gets a default element right in the constructor. I did not want to catch exceptions here to in order to keep the sample code simple.

I put the aspect in another package than the application code in order to demonstrate than generally you need to use fully qualified class names when using annotation-style AspectJ (in native syntax imports would be enough).

Now what is the solution? Basically just this, because the changes mentioned above just make the code better or the test program closer to real-life situations:

@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect { // (...)

Here is the full, refactored sample code:

Application code + driver application:

package de.scrum_master.app;



import java.util.ArrayList;

import java.util.List;



public class MyStructure {

  private String name;

  private List<String> myList;



  public MyStructure(String name) {

    this.name = name;

    myList = new ArrayList<String>();

    myList.add("dummy element to permit reading");

  }



  public void insert(String value) {

    myList.add(value);

  }



  public void read(int pos) {

    myList.get(pos);

  }



  @Override

  public String toString() {

    return "MyStructure[" + name + "]";

  }

}

package de.scrum_master.app;



public class Writer extends Thread {

  private MyStructure myStructure;



  public Writer(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.insert("example");

  }

}

package de.scrum_master.app;



public class Reader extends Thread {

  private MyStructure myStructure;



  public Reader(MyStructure myStructure) {

    this.myStructure = myStructure;

  }



  @Override

  public void run() {

    myStructure.read(0);

  }

}

package de.scrum_master.app;



import java.util.Arrays;

import java.util.Collections;

import java.util.List;



public class Application {

  public static void main(String args) {

    MyStructure structureA = new MyStructure("One");

    MyStructure structureB = new MyStructure("Two");

    List<Thread> threads = Arrays.asList(

      new Writer(structureA), new Writer(structureB), new Writer(structureA), new Writer(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB),

      new Reader(structureA), new Reader(structureB), new Reader(structureA), new Reader(structureB)

    );

    Collections.shuffle(threads);

    for (Thread thread : threads)

      thread.start();

  }

}

Aspect:

package de.scrum_master.aspect;



import java.util.concurrent.locks.Lock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



import org.aspectj.lang.ProceedingJoinPoint;

import org.aspectj.lang.annotation.Around;

import org.aspectj.lang.annotation.Aspect;



import de.scrum_master.app.MyStructure;



@Aspect("pertarget(execution(de.scrum_master.app.MyStructure.new(..)))")

public class LocksAspect {

  private static final long startTime = System.currentTimeMillis();



  private ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock();

  private Lock readLock = readWriteLock.readLock();

  private Lock writeLock = readWriteLock.writeLock();



  @Around("target(myStructure) && execution(void insert(String))")

  public void InsertPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    writeLock.lock();

    log("entering write section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting write section", myStructure);

      writeLock.unlock();

    }

  }



  @Around("target(myStructure) && execution(void read(int))")

  public void ReadPointcutBefore(ProceedingJoinPoint thisJoinPoint, MyStructure myStructure) throws Throwable {

    readLock.lock();

    log("entering read section", myStructure);

    try {

      Thread.sleep(1000);

      thisJoinPoint.proceed();

    } finally {

      log("exiting read section", myStructure);

      readLock.unlock();

    }

  }



  private static void log(String message, Object targetObject)  {

    System.out.printf(

      "%8d ms | %-25s | %-17s | %s%n",

      System.currentTimeMillis() - startTime,

      Thread.currentThread(),

      targetObject,

      message

    );

  }

}

Sample log output:

       4 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | entering write section

       4 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | entering read section

       4 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | entering read section

    1019 ms | Thread[Thread-3,5,main]   | MyStructure[Two]  | exiting write section

    1020 ms | Thread[Thread-8,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-4,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | entering read section

    1020 ms | Thread[Thread-6,5,main]   | MyStructure[One]  | exiting read section

    1020 ms | Thread[Thread-10,5,main]  | MyStructure[One]  | exiting read section

    1025 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | entering write section

    2023 ms | Thread[Thread-11,5,main]  | MyStructure[Two]  | exiting read section

    2024 ms | Thread[Thread-5,5,main]   | MyStructure[Two]  | exiting read section

    2025 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | entering write section

    2026 ms | Thread[Thread-2,5,main]   | MyStructure[One]  | exiting write section

    2026 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | entering write section

    3026 ms | Thread[Thread-1,5,main]   | MyStructure[Two]  | exiting write section

    3026 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | entering read section

    3026 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | entering read section

    3028 ms | Thread[Thread-0,5,main]   | MyStructure[One]  | exiting write section

    4028 ms | Thread[Thread-7,5,main]   | MyStructure[Two]  | exiting read section

    4029 ms | Thread[Thread-9,5,main]   | MyStructure[Two]  | exiting read section

edited Nov 23 '18 at 11:42

answered Nov 22 '18 at 8:19

kriegaex

30.8k363100

edited Nov 23 '18 at 11:42

answered Nov 22 '18 at 8:19

kriegaex

30.8k363100

answered Nov 22 '18 at 8:19

kriegaex

30.8k363100

answered Nov 22 '18 at 8:19

kriegaex

30.8k363100

Minor, unrelated to aspects comment. The idiom (I think) is to acquire a lock, and then enter the try/finally, not try, then acquire, then finally. The idea being that if an exception (esp. InterruptedException) is thrown while acquiring, you do not want to enter the finally clause and release a lock you never successfully acquired in the first place. Please correct me if I'm mistaken.
– GPI
Nov 22 '18 at 8:47

I know that. The try-finally is for calling proceed as well or whatever else could go wrong and I did not want to nest it. You cannot look at code inside an aspect from a non-aspect perspective only. BTW, proceed() can throw Throwable, but whatever happens, the finally block should execute.
– kriegaex
Nov 22 '18 at 14:10

"whatever happens, the finally block should execute". This is where I disagree. If the lock was successfully acquired then the finally block should execute no matter what. But if the lock was not acquired, then the finally block must not be executed (that would release a lock that was never acquired. If your lock is semaphore based, it would release permits that never meant to exist). By placing lock.lock() inside the try, you open up the possibility of failing to acquire and releasing nonetheless, which will at best fail, and at worse trigger unspecified behavior.
– GPI
Nov 23 '18 at 11:25

Yesterday I looked at too quickly, my bad. I am stupid and ugly and you are right. I updated my code accordingly, moving the lock operations to before the respective 'try' blocks. Thanks for your valuable comments! :-)
– kriegaex
Nov 23 '18 at 11:44

Not an issue. I had a feeling we were not understanding each other. Very nice answer, by the way, +1.
– GPI
Nov 23 '18 at 15:51

add a comment |

Minor, unrelated to aspects comment. The idiom (I think) is to acquire a lock, and then enter the try/finally, not try, then acquire, then finally. The idea being that if an exception (esp. InterruptedException) is thrown while acquiring, you do not want to enter the finally clause and release a lock you never successfully acquired in the first place. Please correct me if I'm mistaken.
– GPI
Nov 22 '18 at 8:47

I know that. The try-finally is for calling proceed as well or whatever else could go wrong and I did not want to nest it. You cannot look at code inside an aspect from a non-aspect perspective only. BTW, proceed() can throw Throwable, but whatever happens, the finally block should execute.
– kriegaex
Nov 22 '18 at 14:10

"whatever happens, the finally block should execute". This is where I disagree. If the lock was successfully acquired then the finally block should execute no matter what. But if the lock was not acquired, then the finally block must not be executed (that would release a lock that was never acquired. If your lock is semaphore based, it would release permits that never meant to exist). By placing lock.lock() inside the try, you open up the possibility of failing to acquire and releasing nonetheless, which will at best fail, and at worse trigger unspecified behavior.
– GPI
Nov 23 '18 at 11:25

Yesterday I looked at too quickly, my bad. I am stupid and ugly and you are right. I updated my code accordingly, moving the lock operations to before the respective 'try' blocks. Thanks for your valuable comments! :-)
– kriegaex
Nov 23 '18 at 11:44

Not an issue. I had a feeling we were not understanding each other. Very nice answer, by the way, +1.
– GPI
Nov 23 '18 at 15:51

Minor, unrelated to aspects comment. The idiom (I think) is to acquire a lock, and then enter the try/finally, not try, then acquire, then finally. The idea being that if an exception (esp. InterruptedException) is thrown while acquiring, you do not want to enter the finally clause and release a lock you never successfully acquired in the first place. Please correct me if I'm mistaken.
– GPI
Nov 22 '18 at 8:47

I know that. The try-finally is for calling proceed as well or whatever else could go wrong and I did not want to nest it. You cannot look at code inside an aspect from a non-aspect perspective only. BTW, proceed() can throw Throwable, but whatever happens, the finally block should execute.
– kriegaex
Nov 22 '18 at 14:10

"whatever happens, the finally block should execute". This is where I disagree. If the lock was successfully acquired then the finally block should execute no matter what. But if the lock was not acquired, then the finally block must not be executed (that would release a lock that was never acquired. If your lock is semaphore based, it would release permits that never meant to exist). By placing lock.lock() inside the try, you open up the possibility of failing to acquire and releasing nonetheless, which will at best fail, and at worse trigger unspecified behavior.
– GPI
Nov 23 '18 at 11:25

Yesterday I looked at too quickly, my bad. I am stupid and ugly and you are right. I updated my code accordingly, moving the lock operations to before the respective 'try' blocks. Thanks for your valuable comments! :-)
– kriegaex
Nov 23 '18 at 11:44

Not an issue. I had a feeling we were not understanding each other. Very nice answer, by the way, +1.
– GPI
Nov 23 '18 at 15:51

add a comment |

draft saved

draft discarded

Thanks for contributing an answer to Stack Overflow!

Please be sure to answer the question. Provide details and share your research!

But avoid …

Asking for help, clarification, or responding to other answers.

Making statements based on opinion; back them up with references or personal experience.

To learn more, see our tips on writing great answers.

draft saved

draft discarded

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Name

Required, but never shown

Name

Required, but never shown

This page is only for reference, If you need detailed information, please check here

搜尋此網誌

Ndtyjky