Is this a bug?

[blongstreth]

For Dynamic method return types, I am wondering if you meant to allow a Promise type or any subclass of it? If so, then I think the following code snippet might be a bug: https://github.com/orfjackal/jumi-actors/blob/1e36f97e02171b00548553d8d4b8236fd8e12c1a/jumi-actors/src/main/java/fi/jumi/actors/eventizers/dynamic/DynamicListenerToEvent.java#L28

Should it be as follows? if (Promise.class.isAssignableFrom(method.getReturnType())) {

[luontola]

You can check that by changing the code and running all tests; that change will make ActorsContract.handlers_can_return_arbitrary_futures fail. ;)

It helps to remember than the order of parameters in isAssignableFrom is inverse to the instanceof operator.

[blongstreth]

Thanks for the quick reply! I fully understand the rules and I didn't realize the goal was to allow any subclass of the Future class. Please note that I have not even tried to compile or run tests, I was simply reviewing the source code. My initial thought was you were wanting to pin the base class to Promise.class. For any instance of Future (but not limited to Promise.class), I would have expected the following: if (Future.class.isAssignableFrom(method.getReturnType())) Anyway, Thanks again.

[luontola]

Oh yeah, the Future interface. I forgot about that aspect.

I don't remember why I wrote the code to check for the custom Promise class instead of the Future interface. The way it's currently, instead of the isAssignableFrom method it would be clearer to just use the equals method, because Promise is a final class.

It might be possible to modify the code to support arbitrary Futures, though I'm not sure whether it would complicate things needlessly. I haven't myself used the return values much, so it's hard to say that what kind of an API would be best for the uses cases of this library. Also the Promise class still only has TODOs for exception support, so it's far from feature complete.

I guess I'll go change it to use equals and add some more TODOs about the Feature interface. 😆

[luontola]

Um, actually the original code was correct. It was just very non-obvious. I added a comment for that.

The code checks whether the return type is a supertype of Promise. This matches both Promise and Future. What it doesn't match is e.g. FutureTask. If the code would use if (Future.class.isAssignableFrom(method.getReturnType())) then it would also match methods which return FutureTask, but the code would crash at runtime because this wrapper code will always return a Promise instance to the caller.

The test handlers_can_return_arbitrary_futures checks that the library supports the callee returning any Future implementations, but the declared method return type must be Future so that it would be compatible with Promise.

[luontola]

I just realized that it should disallow Object return types. Maybe I’ll just do an equality check for Promise, Future and ListenableFuture. That should be the most readable version of the code.

[blongstreth]

Sounds reasonable. FYI: I am on Java 8 (CompletableFuture is awesome) and must use the latest released version, so I don't really have anyway to work with the master. Also, the master does have dependencies on guava which has @beta or unstable stuff. If somehow you can fix that sometime would be cool.

Finally,

The other issue I run into which I guess is normal, is that I have an interface which has metadata methods combined with actually methods I want to bind to (proxy). I have had to use trickery and composites to make it work. What would be cool if you could annotate only those methods which are applicable to proxying (if it is even possible). Then it would simplify the code and make the interface more flexible. Example:

interface Foo {
    String getA();
    int getB();
    @Bind
    void onDoSomething(String arg);
 }

Then when binding, it would scan and and only worry about the method with the @Bind annotation. The other methods would be ignored and hopefully not barf the Proxy object.

[luontola]

Guava is shaded inside the released artifact, so it should not leak into your project: https://search.maven.org/remotecontent?filepath=fi/jumi/actors/jumi-actors/1.0.277/jumi-actors-1.0.277.pom

The other issue could also be solved by checking the return type when the method is called, instead of when the actor system is setup. But I prefer failing early. Another option would be to have an alternative factory method for creating DynamicEventizer so that it relaxes those checks. I don't like littering code with annotations, so I would prefer to avoid the need to mark every method (marking the proxied methods or the ignored methods).

Please create a new issue about this need for unrelated methods in the same interface.