Making `SuspendableCallable @Suspendable { .. }` work again with Kotlin #121

aristotaloss · 2015-09-25T14:39:41Z

Since M13 was pushed out the co.paralleluniverse.fibers.Suspendable annotation can no longer be applied to a lambda as it could before because the annotation now needs to have the Kotlin target AnnotationTarget.EXPRESSION or else it will simply not compile. An example would be:

@Suspendable {
    println("Hi")
}

The above does not compile anymore because the annotation misses the target. I replaced the class by a custom variant using the Kotlin expression type:

package co.paralleluniverse.fibers

import kotlin.annotation.Retention
import kotlin.annotation.Target

@Retention // Defaults to RUNTIME
@Target(AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY_GETTER,
        AnnotationTarget.PROPERTY_SETTER, AnnotationTarget.CLASS,
        AnnotationTarget.FILE, AnnotationTarget.EXPRESSION)

annotation public class Suspendable

That does compile fine and results in compilation succeeding for the previously described example. However, replacing the packaged Suspendable annotation with the custom Kotlin variant seems to break the library (it does not detect instrumented methods anymore), so I'm looking for some kind of input on having this nifty feature working again.

Cheers.

The text was updated successfully, but these errors were encountered:

aristotaloss · 2015-09-26T21:30:15Z

I'd be willing to contribute in whatever way with fixing this issue so if anything's needed from my end please do let me hear!

circlespainter · 2015-09-27T03:36:20Z

Can you confirm you're using 0.7.4-SNAPSHOT? What happens if you remove the "@Suspendable" annotation from your lambda? I'll be hearing from the Kotlin team, apart from that I think adding a Kotlin annotation and recognizing it it in the SuspendableClassifier is the way.

aristotaloss · 2015-09-27T09:28:32Z

Yes, I can confirm this. The two jar libraries I use come from the sonatype snapshot repository and are:

quasar-core-0.7.4-20150921.123502-3-jdk8.jar
quasar-kotlin-0.7.4-20150921.123715-3.jar

The Kotlin version I use in my project is 0.13.1514.

Removing it will probably result in it not being instrumented at all, but I will test that and get back to you.

Adding a Kotlin annotation would probably be the best way to go - it leaves the Java annotations as-is which is working all good but if a new annotation would be able to hint the instrumenting then that would be the cleanest solution I think.

When there's anything I need to test furthermore please do let me hear.

aristotaloss · 2015-09-27T09:32:23Z

Regarding removing the annotation: it skips instrumenting the lambda code in general so it doesn' work.

aristotaloss · 2015-09-27T11:09:59Z

I think I'm close to contributing a fix - I've been hacking around to see if I would be able to get it fixed and after adding a secondary annotation in the quasar-kotlin module and making that a recognized instrumentation annotation I think things worked out the way it used to. Currently all tests pass but I need to verify if the inner workings of my project are as with M12.

Once this all works I can submit a pull request.

circlespainter · 2015-09-27T12:16:15Z

Thanks a lot for looking into this!

I'd really like to avoid embedding knowledge about Kotlin in quasar-core though, so my idea was about working at the classifier level (if at all needed).

I've just gone through a review of such classifier and I think the current master should work for you without the @Suspendable annotation. For example this test (as all the other ones) passes fine even with verification turned on. In what is it different from your Kotlin code, can you provide a minimal sample project?

Also, this test will let all inner classes in Kotlin sources proceed to the following logic, which marks for (potential) instrumentation all the methods that implement any of Kotlin runtime's "suspendable-supers" listed in the array (that is, the ones that define Kotlin's callable features in the runtime). This means it should cover lambdas as well (I think they inherit from FunctionN), that is the lambda test in the testsuite and, theoretically, also your lambdas.

aristotaloss · 2015-09-27T12:24:49Z

An example of my use case is this:

    r.onObject(10073) @Suspendable {
        it.player().lock()
        it.delay(1)
        it.player().animate(828, 15)
        it.delay(2)
        it.player().teleport(3102, 3279, 3)
        it.animate(-1)
        it.player().skills().addXp(Skills.AGILITY, 5.0)
        it.player().unlock()
    }

It's a library providing blocking calls for a game. As you can see, a lambda is passed to a repository to assign it to a trigger to run later on when it's triggered. I did actually try as you said without the @Suspendable annotations but it didn't work out.

Is there anything else I'd be able to change to let the pull request be merged? To me it seems like a viable solution because it works fine for projects without quasar-kotlin because it's only a string checking reference for the Kotlin annotation.

circlespainter · 2015-09-27T12:31:10Z

Really strange. Could you provide the stacktrace of your issue with verification turned on, when removing the @Suspendable? The relevant onObject portion calling into the lambda could be useful as well. Of course if onObject calls into the suspendable lamdba, then it must be declared suspendable too (and all the way up the call chain). Can you also confirm it is marked as such?

aristotaloss · 2015-09-27T12:48:26Z

onObject puts the lambda in a map, and a later function calls that lamda. They are all marked as @Suspendable.

    public void onObject(int id, Function1<Script, Unit> script) {
        objectTriggers.put(id, script);
    }

Triggering it does this:

    public boolean triggerObject(Player player, MapObj obj, int action) {
        player.putattrib(AttributeKey.INTERACTION_OBJECT, obj);
        player.putattrib(AttributeKey.INTERACTION_OPTION, action);
        if (objectTriggers.containsKey(obj.id())) {
            executor.executeScript(player, objectTriggers.get(obj.id()));
            return true;
        } else {
            return false;
        }
    }

Which lets the executor invoke the script. That creates a new Fiber and lets the fiber invoke the lambda. The calling part is annotated with @Suspendable which then invokes the lambda stored in the map:

    fun executeScript(context: Any, function: (Script)->Any) {
        var scriptObj = Script(context = context, waitReason = WaitReason.NONE, waitParam = null, fiber = null)
        var fiber = Fiber(SuspendableCallable(@Suspendable {
            function(scriptObj)
        }))
        scriptObj.fiber = fiber
        scriptObj.lastTick = cycle
        fiber.start()

        // Wait until execution stops, and schedule if necessary
        awaitAndSchedule(scriptObj)
    }

circlespainter · 2015-09-27T13:01:12Z

Can you also provide the stacktrace?

aristotaloss · 2015-09-27T13:06:12Z

Going to be a bit harder as I'll have to revert all my fixes and make some changes, but I'll do my best to get this. If I recall correctly, it just throws the well-known "did you forget to instrument something?' with the methods annotated with **s to indicate that instrumenting didn't really work out.

circlespainter · 2015-09-27T13:16:02Z

Thanks. Ok so the code invoking the lambda in executeScript really looks like testFunLambda, do you agree? So I wonder why the test doesn't result in an exception while your code does.

aristotaloss · 2015-09-27T13:24:04Z

Quite interesting indeed, because that's pretty much what my code does too. I'm not sure what the main difference is really. All I know is that it didn't properly instrument my variant.

I'll do some more research, seeing if I can come up with a reason why exactly the way I used the lambdas wasn't working. Thanks for your time 👍

circlespainter · 2015-09-27T13:51:31Z

This is actually more similar to your code:

@Suspendable
private fun callSusLambda(f: (Int) -> Unit, i: Int) =
    Fiber(scheduler, object : SuspendableCallable<Boolean> {
        @Suspendable override fun run(): Boolean {
            f(i)
            return true
        }
    }).start().get()

@Test fun testFunLambda() {
    assertTrue(callSusLambda({ Fiber.sleep(10) }, 1))
}

But it still passes. This is even more similar though:

@Suspendable
private fun callSusLambda(f: (Int) -> Unit, i: Int) =
    Fiber(scheduler, SuspendableCallable {
        f(i)
        true
    }).start().get()

@Test fun testFunLambda() {
    assertTrue(callSusLambda({ Fiber.sleep(10) }, 1))
}

And this one actually breaks with:

co.paralleluniverse.fibers.VerifyInstrumentationException: Target class class co.paralleluniverse.kotlin.fibers.lang.FunTest$callSusLambda$1 has not been instrumented.
    at co.paralleluniverse.fibers.Fiber.verifyInstrumentedTarget(Fiber.java:252)
    at co.paralleluniverse.fibers.Fiber.<init>(Fiber.java:181)
    at co.paralleluniverse.fibers.Fiber.<init>(Fiber.java:332)
    at co.paralleluniverse.kotlin.fibers.lang.FunTest.callSusLambda(FunTest.kt:135)
    at co.paralleluniverse.kotlin.fibers.lang.FunTest.testFunLambda(FunTest.kt:141)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecuter.runTestClass(JUnitTestClassExecuter.java:105)
    at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecuter.execute(JUnitTestClassExecuter.java:56)
    at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassProcessor.processTestClass(JUnitTestClassProcessor.java:64)
    at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:50)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:35)
    at org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
    at org.gradle.messaging.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:32)
    at org.gradle.messaging.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:93)
    at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
    at org.gradle.api.internal.tasks.testing.worker.TestWorker.processTestClass(TestWorker.java:106)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:35)
    at org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
    at org.gradle.messaging.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:360)
    at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:54)
    at org.gradle.internal.concurrent.StoppableExecutorImpl$1.run(StoppableExecutorImpl.java:40)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

I think that class must be an anonymous SuspendableRunnable implementation generated by Kotlin that will end up calling the lambda. My suspicion is that previously the @Suspendable annotation on the expression would carry over to the doRun method implementation but now there isn't the annotation anymore (because it can't be used on expressions).

So there's actually an issue but the scope is narrower than previously thought (that is, interaction with Java8 functional interfaces) and at least there's a workaround: passing an object instance explicitly implementing SuspendableCallable (and declaring it as @Suspendable) rather than letting Kotlin do it (without being able to tell it to carry the annotation over to it anymore, unfortunately).

If you want you could inspect the instrumentation logs and decompile the class (before and/or after instrumentation) to check this is indeed the case. Still if you want, you could try leaving quasar-core intact KotlinClassifier so that it detects the kotlin annotation and returns Suspendable, which I think it's slightly better especially considering the issue has a lesser scope and that any reasonable attempt should be made to let quasar-core be as language-independent as possible.

Thanks for your time!

aristotaloss · 2015-09-27T13:55:24Z

I see now, awesome, glad we were able to narrow this down together. Should I make an effort to edit KotlinClassifier with support for the said annotation with proper Kotlin targeting? It might be easier for me to do it as I've got excessive testing cases and applications with will only work if it all works as expected. I wouldn't mind doing so, but before I do I'd prefer to know if it'd get merged 😉

And I'm really happy to contribute, don't mention it :)

circlespainter · 2015-09-27T14:03:24Z

Contributions are always very welcome although it can require some patience to refine them together to the point they can be included. It's also a further journey to study more of Quasar, if you are interested (and you've got time).

Before you do that though, I'd like first to hear from the Kotlin guys if this M13 limit of Java annotations can be worked around somehow, maybe there's (or there'll be) a way to declare them applicable to expressions. I'll get back here with more insight.

aristotaloss · 2015-09-27T14:08:16Z

I've actually found a member of Kotlin saying that M14 will have this reenabled. This was in a question from someone asking why he couldn't apply an annotation on his lambda: https://devnet.jetbrains.com/message/5558919;jsessionid=65679B2EC5A3E2C88777404D3FF5E9AF

Dmitry Jemerov there replied that M14 will allow the annotations on them again, yet I'm not 100% sure how sure that all is because he mentions the documentation is wrong - giving me the idea that it's going to be a somewhat persisiting change. I guess time or a developer will tell what they will do with it.

And yes, I'm actually very interested in the inner workings and whatnot, and I've got plenty of time hence why I want to help fix this instead of applying the fix to my local project and continuing 😄.

circlespainter · 2015-09-27T14:43:11Z

Thanks for pointing out. That's actually a bit of a strange change also because usually in Kotlin idioms get deprecated (and a refactoring in Idea is provided) before being forbidden altogether.

If it's going to be re-enabled, considering the limited scope of the issue and the fact that there is a workaround, I think probably it's not worth fixing. As for your project you could just use the workaround until M14.

But of course enjoy using (and studying) Quasar!

aristotaloss · 2015-09-27T14:59:42Z

I'm not 100% sure on if it gets re-enabled, but for now I'll just stick to my local edits. If it turns out not to be re-enabled, I'll happily provide the fix in a pull request. If you manage to hear something from the Kotlin team I'd be happy to hear too!

circlespainter · 2015-09-27T15:00:58Z

Sure, will update here, we can leave the PR and the issue open are until more info arrives and we decide what to do.

aristotaloss · 2015-09-27T15:14:23Z

Awesome, I receive notifications on this issue + the PR so when there's any news I'll get back immediately :)

circlespainter · 2015-09-27T21:37:14Z

It looks like the Kotlin change has already been committed and M14 is being released next week (very same thread you cited).

Jire · 2015-09-28T02:30:32Z

@circlespainter That's quite a fast drop of M14. Hopefully the upgrade process in IntelliJ IDEA will be better than what we currently have (not working for almost all of my friends).

aristotaloss · 2015-10-05T09:40:11Z

Hi @circlespainter, to get back to you real quickly after updating to M14 it seems that the change has indeed been made and I can annotate my lambda's like before. A persisting issue I have though is that it seems to fail to be able to check super inheritance when the class is missing on the classpath for whatever reason.

:server:instrument
[ant:quasar] java.lang.NullPointerException
[ant:quasar]    at co.paralleluniverse.fibers.instrument.SimpleSuspendableClassifier.extendsOrImplements(SimpleSuspendableClassifier.java:197)
[ant:quasar]    at co.paralleluniverse.fibers.instrument.SimpleSuspendableClassifier.extendsOrImplements(SimpleSuspendableClassifier.java:183)
[ant:quasar]    at co.paralleluniverse.kotlin.KotlinClassifier.isSuspendable(KotlinClassifier.java:85)
[ant:quasar]    at co.paralleluniverse.fibers.instrument.DefaultSuspendableClassifier.isSuspendable(DefaultSuspendableClassifier.java:41)
[ant:quasar]    at co.paralleluniverse.fibers.instrument.InstrumentClass.visitMethod(InstrumentClass.java:147)
[ant:quasar]    at co.paralleluniverse.asm.ClassReader.b(Unknown Source)
[ant:quasar]    at co.paralleluniverse.asm.ClassReader.accept(Unknown Source)
[ant:quasar]    at co.paralleluniverse.asm.ClassReader.accept(Unknown Source)
[ant:quasar]    at co.paralleluniverse.fibers.instrument.QuasarInstrumentor.instrumentClass(QuasarInstrumentor.java:113)
[ant:quasar]    at co.paralleluniverse.fibers.instrument.QuasarInstrumentor.instrumentClass(QuasarInstrumentor.java:92)
[ant:quasar]    at co.paralleluniverse.fibers.instrument.InstrumentationTask.instrumentClass(InstrumentationTask.java:190)
[ant:quasar]    at co.paralleluniverse.fibers.instrument.InstrumentationTask.execute(InstrumentationTask.java:177)

I'm not sure if this is already fixed on master, but it might be an idea otherwise to make it print a warning if the resolving failed and return false in the method. Just my two cents :).

aristotaloss · 2015-10-05T10:07:24Z

A note on the above: I was missing a kotlin compiler library which resulted in a missing class. Still, it might be very useful to know which class it misses when that method fails :-).

…ll` as per #121

circlespainter · 2015-10-05T16:14:39Z

@Velocity- Will add an assertion w/message about that for now, you can enable it during development with the -ea JVM flag.

circlespainter · 2015-10-05T16:59:31Z

A new 0.7.4-SNAPSHOT is at SonaType, thanks!

circlespainter self-assigned this Sep 25, 2015

aristotaloss mentioned this issue Sep 27, 2015

Make @Suspendable work on lambdas again in Kotlin M13 #122

Closed

circlespainter pushed a commit that referenced this issue Sep 27, 2015

'quasar-kotlin': add info about #121

2ae2595

circlespainter changed the title ~~Making @Suspendable { .. } work again with Kotlin~~ Making SuspendableCallable @Suspendable { .. } work again with Kotlin Sep 27, 2015

circlespainter added the bug label Sep 28, 2015

circlespainter pushed a commit that referenced this issue Oct 5, 2015

Add assertion about db.getOrLoadClassEntry(className) returning `nu…

795e452

…ll` as per #121

circlespainter closed this as completed in 401c356 Oct 5, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making `SuspendableCallable @Suspendable { .. }` work again with Kotlin #121

Making `SuspendableCallable @Suspendable { .. }` work again with Kotlin #121

aristotaloss commented Sep 25, 2015

aristotaloss commented Sep 26, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

Jire commented Sep 28, 2015

aristotaloss commented Oct 5, 2015

aristotaloss commented Oct 5, 2015

circlespainter commented Oct 5, 2015

circlespainter commented Oct 5, 2015

Making SuspendableCallable @Suspendable { .. } work again with Kotlin #121

Making SuspendableCallable @Suspendable { .. } work again with Kotlin #121

Comments

aristotaloss commented Sep 25, 2015

aristotaloss commented Sep 26, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

aristotaloss commented Sep 27, 2015

circlespainter commented Sep 27, 2015

Jire commented Sep 28, 2015

aristotaloss commented Oct 5, 2015

aristotaloss commented Oct 5, 2015

circlespainter commented Oct 5, 2015

circlespainter commented Oct 5, 2015

Making `SuspendableCallable @Suspendable { .. }` work again with Kotlin #121

Making `SuspendableCallable @Suspendable { .. }` work again with Kotlin #121