Any extension that calls `invocation.proceed()` multiple times like `@Retry` misbehaves

[Vampire]

Describe the bug

MethodInvocation has an Iterator for interceptors.
If you call invocation.proceed(), then the iterator is queried for the next interceptor which is then invoked.
So if you call invocation.proceed() a second or further time, all subsequent iterators for that method invocation are not invoked.

MethodInvocation is pretty thin state-wise.
For most things it is just a data-holder.
The only things that are directly stateful are when setArguments(...) is called and when proceed() is called.

I guess the most proper solution would be to replace this.arguments = arguments; by System.arraycopy(arguments, 0, this.arguments, 0, arguments.length); and getting rid of the iterator for interceptors, but instead having an own MethodInvocation for each interceptor that knows the next interceptor to call.

This should be a backwards compatible change and fix all extensions that might do something similar to what @Retry does.

To Reproduce

• Have a global extension (don't forget to register it in META-INF/services) like

  class GlobalRetryExtension implements IGlobalExtension {
     private final Retry retry = Retried.getAnnotation(Retry)
  
     private final RetryExtension extension = new RetryExtension()
  
     @Override
     void visitSpec(SpecInfo spec) {
        if (!spec.getAnnotation(Retry)) {
           extension.visitSpecAnnotation(retry, spec)
        }
     }
  
     @Retry(mode = SETUP_FEATURE_CLEANUP)
     private static final class Retried {
     }
  }

• Have a specification with a @TempDir field
• Make sure the specification fails so that it is retried

Now either set breakpoints in the RetryIterationInterceptor and TempDirInterceptor, or check in the feature the presence of the temp dir, or similar.

Expected behavior

Retrying (and any interceptors doing something similar) should properly invoke subsequent interceptors each time.

Actual behavior

With non-suspending breakpoints that do some logging on the two mentioned interceptors, one on the proceed() line and one on the following line for each, you get output like:

retry try
temp dir: .../spock_build_should_fail_i_0_rootDir6715396349974787942
temp dir destroy:.../spock_build_should_fail_i_0_rootDir6715396349974787942
retry after try
retry try
retry after try
retry try
retry after try
retry try
retry after try

Dependencies

Spock 2.3-groovy-3.0

[Vampire]

I just was hit by this again in another context.

I have a GlobalRetryExtension that applies the Retry extension on all specs.
And I have geb-spock with its global GebExtension.
As I'm testing a Swing-application with that using Marathon Java driver, I have disabled caching of the driver, as each test iteration needs to start a new driver.
This causes the GebExtension to call resetBrowser() which it does in an iteration interceptor.
But even with the @Retry being done with SETUP_FEATURE_CLEANUP this does not work properly, because on first execution, the Geb iteration interceptor that calls resetBrowser() after the test is invoked properly.
But on every further retry the interceptor is not invoked, causing the browser not being reset properly.

[leonard84]

Describe the bug

MethodInvocation has an Iterator for interceptors. If you call invocation.proceed(), then the iterator is queried for the next interceptor which is then invoked. So if you call invocation.proceed() a second or further time, all subsequent iterators for that method invocation are not invoked.

MethodInvocation is pretty thin state-wise. For most things it is just a data-holder. The only things that are directly stateful are when setArguments(...) is called and when proceed() is called.

I guess the most proper solution would be to replace this.arguments = arguments; by System.arraycopy(arguments, 0, this.arguments, 0, arguments.length); and getting rid of the iterator for interceptors, but instead having an own MethodInvocation for each interceptor that knows the next interceptor to call.

It won't fix the ordering problem. For this to work properly, the retrying extension must be the outermost extension.

[Vampire]

Well, depends on what should be retried and is more the topic of being able to define extensions order / dependencies which is in another issue.

The problem is, that currently the interceptors that are "inside" the retry interceptor are not taking part in the retrying which simply is wrong behavior, see the two examples.

[AndreasTu]

@Vampire Do you think this should still hit the 2.4 release?

[Vampire]

I really hope so.
This is a really annoying and sneaky problem and when you hit it you every time need a looong time to figure out what the damn problem might be.

[AndreasTu]

I do not really get the following part:

replace this.arguments = arguments; by System.arraycopy(arguments, 0, this.arguments, 0, arguments.length);

Isn't there maybe a case where multiple interceptors want to manipulate the arguments array and the next want to consume it again.
So why would we need to copy the arguments array for each interceptor?
Why can't we wire the same argument array from the previous interceptor into the next one?

[Vampire]

I have no idea, when I wrote that almost 2 years ago I for sure had a reason to write it. :-D

[Vampire]

Also, the interceptors do not get an argument array.
The interceptors get the MethodInvocation and on that can call getArguments, setArguments and resolveArgument.
In Spock itself we never call setArguments.

Reading through the code and my comment above, I guess the idea was, if we split the MethodInvocation to one per interceptor that knows the next interceptor, and an interceptor is calling setArguments it should not propagate or something like that.