Fix @DeclareMixin race condition during delegate member initialisation

[kriegaex]

Fix #198 and add a regression test for it.

@aclement, please review and post some feedback, especially concerning the following two issues:

1. I am synchronising on this in order to avoid adding an extra private Object syncOnMe = new Object(); helper field to synchronise on in each target class. Is that too dangerous for target class performance? I tried making the synchronisation as fine-grained as possible, i.e. only around the actual lazy delegate field initialisation as follows:

     public void methodOne() {
       synchronized(this) {
         if (this.ajc$instance$X$I == null) {
           this.ajc$instance$X$I = X.aspectOf().createImplementation(this);
         }
       }
   
       this.ajc$instance$X$I.methodOne();
     }

   BTW, we cannot synchronise on the delegate field itself, because it might still be null, which would lead to runtime exceptions.

2. Instead of the tedious synchronisation, we could also initialise each delegate during declaration, i.e. the byte code equivalent of private I ajc$instance$X$I = X.aspectOf().createImplementation(this);. Byte code generation in the weaver would be much easier, but we would lose the advantage of lazy initialisation.

[kriegaex]

In https://github.com/eclipse/org.aspectj/actions/runs/3801161882, we see that the new regression test reproduces the concurrency problem:

Error:  testCaseEConcurrent(org.aspectj.systemtest.ajc164.DeclareMixinTests)  Time elapsed: 0.303 s  <<< FAILURE!
junit.framework.AssertionFailedError: 

  expecting output:
Delegate factory invoked for CaseEConcurrent instance: a
Delegate factory invoked for CaseEConcurrent instance: b
methodOne running on CaseEConcurrent instance: a
methodTwo running on CaseEConcurrent instance: a
methodOne running on CaseEConcurrent instance: b
methodTwo running on CaseEConcurrent instance: b
  but found output:
Delegate factory invoked for CaseEConcurrent instance: a
Delegate factory invoked for CaseEConcurrent instance: b
Delegate factory invoked for CaseEConcurrent instance: a
Delegate factory invoked for CaseEConcurrent instance: b
methodTwo running on CaseEConcurrent instance: a
methodOne running on CaseEConcurrent instance: a
methodOne running on CaseEConcurrent instance: b
methodTwo running on CaseEConcurrent instance: b

Expected 6 lines of output but there are 8

    at org.aspectj.systemtest.ajc164.DeclareMixinTests.testCaseEConcurrent(DeclareMixinTests.java:73)

In build https://github.com/eclipse/org.aspectj/actions/runs/3801947405, however, that test passes. So do all the other tests. In a previous commit, overwritten by force-push, there was a bug in my change which made some other tests fail, but now that one is also fixed.

[kriegaex]

@aclement, I would really have felt much better with a thorough or at least cursory code review and answers to my questions above. However, I do not want to wait for a long time before merging fixes. Please, by all means do review this fix post factum. I am ready to improve or revert it, if necessary.

[aclement]

I just wonder if worth wrapping with an extra null check:

if (this.ajc$instance$X$I == null) {
  synchronized(this) {
    if (this.ajc$instance$X$I == null) {
      this.ajc$instance$X$I = X.aspectOf().createImplementation(this);
    }
  }
}

Since the common case is that it is not null if exercising these mixins, and so you'd avoid synchronizing once it is initialized. But maybe the JVM is smart enough to recognize this situation these days.

[kriegaex]

@aclement: What is the rationale behind your idea? My educated guess would be that you want to avoid the ITD method from having to wait for the lock in cases where the member already has a value. If so, I can look into it, even though byte code generation is a pain in the neck and it takes me a long time to get it right even in the simplest cases.

[aclement]

want to avoid the ITD method from having to wait for the lock in cases where the member already has a value

exactly, as I wrote in my description "avoid synchronizing once it is initialized"

[kriegaex]

@aclement: Yeah, sorry for effectively just paraphrasing what you said. Your comment was terse, and I was unsure if that was what you meant. Anyway, I implemented your idea in 83331e4b. Your feedback is most welcome and super important to me, because you are clearly my senior in AspectJ maintenance, and I am not much more than a byte code generation noob anyway.