Instrumentation verification doesn't work in this use case

[ufoscout]

Hi all,

playing with quasar I found a strange and wrong behaviour.

Look at this simple unit test:

    @Test
    public void testQuasar() throws ExecutionException, InterruptedException {

        AsyncApi async = new AsyncApiImpl();
        SyncApi sync = new SyncApiImpl(async);

        new Fiber<Void>(() -> {

            int a = new Random().nextInt(1000);
            System.out.println("a is " + a);

            int b = new Random().nextInt(1000);
            System.out.println("b is " + b);

            System.out.println(a + " + " + b + " = " + sync.sum(a, b));

        }).start().join();
    }

The AsyncApi is an asynchronous api that returns a CompletableFuture with the sum of two integers.
The SyncApi uses the AsyncCompletionStage.get() method to wrap the AsyncApi call.
When the test is executed, surprisingly, the result is:

a is 235
b is 97
received a is 235
received b is 97
received 235 97 -> returning: 332
a is 778
b is 977
778 + 977 = 332

As you can see the 'a' and 'b' variables are created twice and the related "system.out.println()" calls are also executed two times.

The expected output should have been:

a is 235
b is 97
received a is 235
received b is 97
received 235 97 -> returning: 332
235 + 97 = 332

Here a reproducer for the issue (clone and execute 'mvn clean test' to reproduce the issue):
https://github.com/ufoscout/quasar-test

My OS is ubuntu 15.04 and the JDK is:
java version "1.8.0_25"
Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)

[circlespainter]

You need to declare your interface method SyncApi.sum() (or the whole interface) as @Suspendable, else Quasar can't see that the sync.sum(a, b) call site is a suspendable one and won't instrument it; since it will find no other suspendable calls either, Quasar will end up skipping instrumentation altogether for the whole method, which means the call will be restarted upon execution resume and that's why you see the variables being re-created.

Verification should point it out though and it doesn't instead, this is something I'll look into.

[pron]

@circlespainter Note that verification isn't actually turned on (in spite of the argLine element; Maven thing?) To turn it on, uncomment the systemPropertyVariables section. Although, even if turned on, it doesn't point out this problem.

[circlespainter]

@pron Right, but even when uncommenting it (so that the warning about the instrumentation verification being enabled is printed out) the issue is not detected.

[ufoscout]

Indeed, placing the @Suspendable annotation on the interface solved the issue.
Nevertheless, this silent behaviour is extremely dangerous from my point of view. I could easily forget an annotation and the fact that the application runs with a completely wrong runtime behaviour is quite scaring. Honestly, I expected Quasar to throw an exception and to immediately stop the execution instead.

[circlespainter]

This is what instrumentation verification is for (in addition to tests); this is a very unfortunate (and rare) case where an uninstrumented call site goes undetected by instrumentation verification. We're already working on an improvement that will fix this.

Instrumentation verification is expensive though, that's why it's not always enabled by default and it should be used only during development.

Starting with Java 9 instrumentation will be automatic, with no need for manual annotation anymore.

[circlespainter]

In the meanwhile there's also a scanner task that will detect suspendables: http://docs.paralleluniverse.co/quasar/#advanced-fibers ("Automatic Instrumentation" sub-section). Pulsar (Quasar's bindings and idiomatic API for Clojure) also supports automatic instrumentation.

[ufoscout]

Starting with Java 9 instrumentation will be automatic, with no need for manual annotation anymore.

So cool!!!

[pron]

So cool!!!

That's not entirely up to us. We've helped with a small change to HotSpot that will make this possible. Oracle has not yet confirmed that the change will be merged in Java 9, but the chances are good.