review Java usage of ActorSystem

[pjfanning]

See #1419 (comment)

terminate() returns a Scala Future

In theory, users can ignore the return value from terminate() and call this:
https://pekko.apache.org/japi/pekko/current/org/apache/pekko/actor/ActorSystem.html#getWhenTerminated()

Maybe, we could also add terminateWithCompletionStage that returns CompletionStage<Terminated>?

Interestingly, the actor-typed ActorSystem has a void return type on terminate().
You have https://pekko.apache.org/japi/pekko/current/org/apache/pekko/actor/typed/ActorSystem.html#getWhenTerminated() for Java users.

[pjfanning]

@mdedetrich @raboof @He-Pin My view is that it's not very useful to make changes. Java users can ignore the return value on the class ActorSystem terminate() call and then use the getWhenTerminated() method to get a CompletionStage to check. This maps closely to what they would do with the typed ActorSystem too (where terminate() returns a void anyway).

[He-Pin]

Not sure about this, there is an Futures.asJava help method, maybe we can add gracefulTerminate(timeout, timeunit) method as Netty？I think that can cover 80％ cases

[mdedetrich]

@mdedetrich @raboof @He-Pin My view is that it's not very useful to make changes. Java users can ignore the return value on the class ActorSystem terminate() call and then use the getWhenTerminated() method to get a CompletionStage to check. This maps closely to what they would do with the typed ActorSystem too (where terminate() returns a void anyway).

Ignoring the Future from .terminate() call shouldn't be encouraged, because if you need to do something after the Actor termination then the termination will only occur successful when that Future completes.

When using the java API I actually have to use .toCompletableFuture() because of this, i.e.

fun shutdown() {
    logger.info("Terminating actor system")
    FutureConverters.asJava(system.terminate()).toCompletableFuture().whenComplete { _, exception ->
        if (exception != null) {
            logger.warn("Error terminating actor system", exception)
        } else
            logger.info("Terminated actor system")
    }.get()
}

Its also not consistent, i.e. if you are using pekko http its standard practice to terminate the actor system after you terminate the http server, where again you have to call the .toCompletableFuture() method when using java api

serverBinding.terminate(Duration.ofSeconds(10)).thenCompose {
  FutureConverters.asJava(system.terminate()).toCompletableFuture()
}

its all really quite ugly, I would personally push to have a proper terminate method for the Java API which returns a CompletableFuture. 2.0.x allows for breaking changes, so now is the opportunity to do such a change

[pjfanning]

@mdedetrich I would prefer that we keep the existing ActorSystem def terminate(): Future[Terminated].
Can we just add def terminateWithCompletionStage(): CompletionStage[Terminated]?

[mdedetrich]

@mdedetrich I would prefer that we keep the existing ActorSystem def terminate(): Future[Terminated].

Well one way to solve this issue would be to provide an API that accepts an ActorSystem as an argument, that way you would have a pekko.scaladsl.actor.Terminate(actorSystem: ActorSystem) which will return Future[Terminated] and you would have a pekko.actor.javadsl.Terminate(actorSystem: ActorSystem)which will return aCompletionStage[Terminated]`.

There are even more natural options than this, one is to make our current ActorSystem private/internal stable API (for 2.0.x), and create an ActorSystem in both the javadsl and scaladsl package that inherits the current ActorSystem and have all of the Future returning methods on the scaladsl Actor and all of the CompletionStage ones on the javadsl actor. Another alternative is to use the delegate pattern which is also frequently used in pekko codebase.

In conjunction you would also deprecate the current def terminate(): Future[Terminated] on ActorSystem in 1.2.x.

I know that this is more work, however the counter argument to that is that the current terminate() method is clearly inconsistent with how the rest of the pekko API's are structured (where the api's are split between the scaladsl and javadsl packages respectively) and hence its current design seems to be a mistake/oversight?

Can we just add def terminateWithCompletionStage(): CompletionStage[Terminated]?

This would be out of place in terms of the rest of the pekko api's are designed.

I have a bit of time at the end of the week, I can create a PR with one of the implementations I suggested (likely would be inheritence version since ActorSystem is abstract anyways and as a bonus these changes wouldn't break binary compatibility so they can be implemented in 1.2.x (and we would only make the abstract ActorSystem private in 2.0.x). We would howeer have to wait for the 1.2.x release a bit longer in such as a case as I would have to mark some current methods for deprecation

[pjfanning]

I'm not at all sure that I like a major refactor just because a Java friendly terminate isn't implemented.
Can we at least review the API on the typed ActorSystem which is different from the class ActorSystem? The diff being that terminate returns void/unit.
And then users need to call getWhenTerminated() (Java) or whenTerminated() (Scala) to get a CompletionStage or Future as required. These 2 methods are on the classic ActorSystem too.

I think we should try to still bear in mind that it is good to keep the disruption to migrating users to a minimum.

[mdedetrich]

I'm not at all sure that I like a major refactor just because a Java friendly terminate isn't implemented. Can we at least review the API on the typed ActorSystem which is different from the class ActorSystem? The diff being that terminate returns void/unit. And then users need to call getWhenTerminated() (Java) or whenTerminated() (Scala) to get a CompletionStage or Future as required. These 2 methods are on the classic ActorSystem too.

I think we should try to still bear in mind that it is good to keep the disruption to migrating users to a minimum.

The thing is its not just terminate that returns a Future, as you stated there is also whenTerminated but that is still sticking out as a sore thumb. But you are right in that I should look into the typed ActorSystem and see the reasoning/intent behind it returning Void/Unit (I guess they made that method blocking to make it easier to use, although if thats the case that is a not very akka/pekko like???)

Can we hold off on the 1.2.x release until I look into this properly? There is another reason behind this change but I first need to look into how many divergent API's we have (there is a big difference if the only divergence we have is these 2 methods vs a lot of others).

[He-Pin]

@pjfanning @mdedetrich Add a terminateWithCompletionStage is ok too, as there is something like that everywhere:

  def invokeWithFeedbackCompletionStage(t: T): CompletionStage[Done] = {
    import pekko.util.FutureConverters._
    invokeWithFeedback(t).asJava
  }

[He-Pin]

If we want to do this in a great way for Java, we should reimplement all Java interfaces under javadsl and expose no Scala types, which can be done in 2.0.0, and provide asJava and asScala to convert bidirectionally.

And this method is being used very low frequently, so I suggest we just release the 1.2.0 and then do a better shaping in 2.0.0

A great Java api will need to restructure the current api/implementation.

[He-Pin]

@mdedetrich separating two javadsl and scaladsl will cause some problems too, when you do searching for the same type in the IDE, then two classes show up, you have to select one, select two methods, or select two classes.

As the terminate method is being used very low frequency, we can just leave it as it is.

in 1.2.0:

• leave it as it is

in 2.0.0

• change the result type to Unit and ask users to use whenTerminate/getWhenTerminated

@mdedetrich @pjfanning

[mdedetrich]

@pjfanning @mdedetrich Add a terminateWithCompletionStage is ok too, as there is something like that everywhere:

  def invokeWithFeedbackCompletionStage(t: T): CompletionStage[Done] = {
    import pekko.util.FutureConverters._
    invokeWithFeedback(t).asJava
  }

This is just a bandaid. The ideal state is that for the javadsl, no method accepts/returns a Future and for a scaladsl no method accepts/returns a CompletionStage.

I will work on a PR later on in the week and also explain my intentions (I want to work on a kotlindsl and that only works when we have properly split out dsl's)

[mdedetrich]

As the terminate method is being used very low frequency, we can just leave it as it is.

Where is the evidence of this? In order to properly terminate the actor system without creating spurious warnings and/or make sure its properly shut down in terms of ungraceful shutdown, you should use terminate(). Every akka/pekko codebase I have worked on uses it.

[mdedetrich]

@mdedetrich separating two javadsl and scaladsl will cause some problems too, when you do searching for the same type in the IDE, then two classes show up, you have to select one, select two methods, or select two classes.

Considering that almost every single other part of the akka/pekko codebase does this where you have this problem, we are so far down that I fail to see how this is a good counter argument

In pekko http, almost every single type/class has a Scala and a Java equivalent