Copilot commented on code in PR #2479:
URL: https://github.com/apache/pekko/pull/2479#discussion_r2518004909
##########
actor/src/main/scala/org/apache/pekko/actor/ActorSystem.scala:
##########
@@ -697,7 +710,56 @@ abstract class ActorSystem extends ActorRefFactory with
ClassicActorSystemProvid
* such as `thenRunAsync`, on the dispatchers (`Executor`) of this actor
system as they
* will have been shut down before this CompletionStage completes.
*/
- def getWhenTerminated: CompletionStage[Terminated]
+ @deprecated(
+ "Use the .getWhenTerminated() function on an ActorSystem created by
org.apache.pekko.actor.javadsl.ActorSystem.create",
+ "1.3.0")
+ def getWhenTerminated: CompletionStage[Terminated] =
whenTerminatedImpl.asJava
+
+ /**
+ * Terminates this actor system by running [[CoordinatedShutdown]] with
reason
+ * [[CoordinatedShutdown.ActorSystemTerminateReason]]. This method will block
+ * until either the actor system is terminated or
+ * `pekko.coordinated-shutdown.close-actor-system-timeout` timeout duration
is
+ * passed, in which case a [[TimeoutException]] is thrown.
+ *
+ * If `pekko.coordinated-shutdown.run-by-actor-system-terminate` is
configured to `off`
+ * it will not run `CoordinatedShutdown`, but the `ActorSystem` and its
actors
+ * will still be terminated.
+ *
+ * This will stop the guardian actor, which in turn
+ * will recursively stop all its child actors, and finally the system
guardian
+ * (below which the logging actors reside) and then execute all registered
+ * termination handlers (see [[ActorSystem#registerOnTermination]]).
+ * @since 1.3.0
+ */
+ @throws(classOf[TimeoutException])
+ override def close(): Unit = {
+ terminateImpl()
+ Await.ready(whenTerminatedImpl,
+
Duration(settings.config.getDuration("coordinated-shutdown.close-actor-system-timeout").toMillis,
Review Comment:
The configuration path is missing the 'pekko.' prefix. It should be
'pekko.coordinated-shutdown.close-actor-system-timeout' to match the
configuration added in reference.conf.
```suggestion
Duration(settings.config.getDuration("pekko.coordinated-shutdown.close-actor-system-timeout").toMillis,
```
##########
actor/src/main/scala/org/apache/pekko/actor/ActorSystem.scala:
##########
@@ -526,7 +536,7 @@ object ActorSystem {
* extending [[pekko.actor.ExtendedActorSystem]] instead, but beware that you
* are completely on your own in that case!
*/
-abstract class ActorSystem extends ActorRefFactory with
ClassicActorSystemProvider {
+trait ActorSystem extends ActorRefFactory with ClassicActorSystemProvider with
AutoCloseable {
Review Comment:
Making ActorSystem extend AutoCloseable is a behavior change that could
affect existing code, especially when ActorSystem instances are used in
try-with-resources blocks in Java. The PR description mentions this may be too
much for a non-breaking change and should wait for Pekko 2.0.0. Consider
removing AutoCloseable from this trait.
```suggestion
trait ActorSystem extends ActorRefFactory with ClassicActorSystemProvider {
```
##########
actor/src/main/scala/org/apache/pekko/actor/ActorSystem.scala:
##########
@@ -677,7 +687,10 @@ abstract class ActorSystem extends ActorRefFactory with
ClassicActorSystemProvid
* using the dispatcher of this actor system as it will have been shut down
before the
* future completes.
*/
- def terminate(): Future[Terminated]
+ @deprecated(
+ "Use .close() or the .terminateAsync() function on an ActorSystem created
by org.apache.pekko.actor.scaladsl.ActorSystem.apply",
Review Comment:
[nitpick] The deprecated terminate() method returns Future[Terminated], but
the scaladsl.ActorSystem trait's terminateAsync() also returns
Future[Terminated]. This creates ambiguity - users calling terminate() on a
scaladsl.ActorSystem will still get the same return type. Consider making the
deprecation message clearer about when each method should be used.
```suggestion
"DEPRECATION: Both terminate() and scaladsl.ActorSystem.terminateAsync()
return Future[Terminated], which can cause ambiguity. " +
"If you are using org.apache.pekko.actor.scaladsl.ActorSystem, prefer
.terminateAsync(). Otherwise, use .close(). " +
"This method may be removed in a future release.",
```
##########
actor/src/main/scala/org/apache/pekko/actor/scaladsl/ActorSystem.scala:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2009-2022 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.actor.scaladsl
+
+import scala.concurrent.{ ExecutionContext, Future }
+
+import com.typesafe.config.{ Config, ConfigFactory }
+
+import org.apache.pekko
+import pekko.actor.ActorSystem.findClassLoader
+import pekko.actor._
+import pekko.actor.setup.ActorSystemSetup
+
+trait ActorSystem extends org.apache.pekko.actor.ActorSystem {
+
+ /**
+ * Asynchronously terminates this actor system by running
[[CoordinatedShutdown]] with reason
+ * [[CoordinatedShutdown.ActorSystemTerminateReason]].
+ *
+ * If `pekko.coordinated-shutdown.run-by-actor-system-terminate` is
configured to `off`
+ * it will not run `CoordinatedShutdown`, but the `ActorSystem` and its
actors
+ * will still be terminated.
+ *
+ * This will stop the guardian actor, which in turn
+ * will recursively stop all its child actors, and finally the system
guardian
+ * (below which the logging actors reside) and then execute all registered
+ * termination handlers (see [[ActorSystem.registerOnTermination]]).
+ * Be careful to not schedule any operations on completion of the returned
future
+ * using the dispatcher of this actor system as it will have been shut down
before the
+ * future completes.
+ */
+ def terminateAsync(): Future[Terminated] = terminateImpl()
+
+ /**
+ * Returns a [[Future]] which will be completed after the [[ActorSystem]]
has been terminated
+ * and termination hooks have been executed. If you registered any callback
with
+ * [[ActorSystem.registerOnTermination]], the returned Future from this
method will not complete
+ * until all the registered callbacks are finished. Be careful to not
schedule any operations,
+ * such as `onComplete`, on the dispatchers (`ExecutionContext`) of this
actor system as they
+ * will have been shut down before this future completes.
+ */
+ override def whenTerminated: Future[Terminated] = whenTerminatedImpl
+}
+
+object ActorSystem {
+
+ /**
+ * Creates a new ActorSystem with the name "default",
+ * obtains the current ClassLoader by first inspecting the current threads'
getContextClassLoader,
+ * then tries to walk the stack to find the callers class loader, then falls
back to the ClassLoader
+ * associated with the ActorSystem class.
+ * Then it loads the default reference configuration using the ClassLoader.
+ */
+ def apply(): ActorSystem = apply("default")
+
+ /**
+ * Creates a new ActorSystem with the specified name,
+ * obtains the current ClassLoader by first inspecting the current threads'
getContextClassLoader,
+ * then tries to walk the stack to find the callers class loader, then falls
back to the ClassLoader
+ * associated with the ActorSystem class.
+ * Then it loads the default reference configuration using the ClassLoader.
+ */
+ def apply(name: String): ActorSystem = apply(name, None, None, None)
+
+ def apply(name: String, setup: ActorSystemSetup): ActorSystem = {
+ val bootstrapSettings = setup.get[BootstrapSetup]
+ val cl =
bootstrapSettings.flatMap(_.classLoader).getOrElse(findClassLoader())
+ val appConfig =
bootstrapSettings.flatMap(_.config).getOrElse(ConfigFactory.load(cl))
+ val defaultEC = bootstrapSettings.flatMap(_.defaultExecutionContext)
+
+ val impl = new ActorSystemImpl(name, appConfig, cl, defaultEC, None,
setup) with ActorSystem {
+ // TODO: Remove in Pekko 2.0.0, not needed anymore
+ override def whenTerminated: Future[Terminated] =
super[ActorSystem].whenTerminated
+ }
Review Comment:
[nitpick] The override of whenTerminated is redundant since the trait
already provides this implementation. This TODO suggests it's temporary
scaffolding - consider whether this override is actually needed.
##########
actor/src/main/scala/org/apache/pekko/actor/ActorSystem.scala:
##########
@@ -697,7 +710,56 @@ abstract class ActorSystem extends ActorRefFactory with
ClassicActorSystemProvid
* such as `thenRunAsync`, on the dispatchers (`Executor`) of this actor
system as they
* will have been shut down before this CompletionStage completes.
*/
- def getWhenTerminated: CompletionStage[Terminated]
+ @deprecated(
+ "Use the .getWhenTerminated() function on an ActorSystem created by
org.apache.pekko.actor.javadsl.ActorSystem.create",
+ "1.3.0")
+ def getWhenTerminated: CompletionStage[Terminated] =
whenTerminatedImpl.asJava
+
+ /**
+ * Terminates this actor system by running [[CoordinatedShutdown]] with
reason
+ * [[CoordinatedShutdown.ActorSystemTerminateReason]]. This method will block
+ * until either the actor system is terminated or
+ * `pekko.coordinated-shutdown.close-actor-system-timeout` timeout duration
is
+ * passed, in which case a [[TimeoutException]] is thrown.
+ *
+ * If `pekko.coordinated-shutdown.run-by-actor-system-terminate` is
configured to `off`
+ * it will not run `CoordinatedShutdown`, but the `ActorSystem` and its
actors
+ * will still be terminated.
+ *
+ * This will stop the guardian actor, which in turn
+ * will recursively stop all its child actors, and finally the system
guardian
+ * (below which the logging actors reside) and then execute all registered
+ * termination handlers (see [[ActorSystem#registerOnTermination]]).
+ * @since 1.3.0
+ */
+ @throws(classOf[TimeoutException])
+ override def close(): Unit = {
+ terminateImpl()
+ Await.ready(whenTerminatedImpl,
+
Duration(settings.config.getDuration("coordinated-shutdown.close-actor-system-timeout").toMillis,
+ TimeUnit.MILLISECONDS))
+ }
+
+ /**
+ * Asynchronously terminates this actor system by running
[[CoordinatedShutdown]] with reason
+ * [[CoordinatedShutdown.ActorSystemTerminateReason]]. This method will block
+ * until either the actor system is terminated or
+ * `pekko.coordinated-shutdown.close-actor-system-timeout` timeout duration
is
+ * passed, in which case a [[TimeoutException]] is thrown.
Review Comment:
The documentation for closeAsync() incorrectly states 'This method will
block', but this method is non-blocking and returns Unit immediately without
waiting for termination.
```suggestion
* Asynchronously initiates termination of this actor system by running
[[CoordinatedShutdown]] with reason
* [[CoordinatedShutdown.ActorSystemTerminateReason]]. This method is
non-blocking and returns immediately
* after starting the termination process; it does not wait for the actor
system to terminate.
```
##########
actor/src/main/scala/org/apache/pekko/actor/javadsl/ActorSystem.scala:
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2009-2022 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.actor.javadsl
+
+import java.util.concurrent.CompletionStage
+
+import scala.concurrent.ExecutionContext
+
+import com.typesafe.config.{ Config, ConfigFactory }
+
+import org.apache.pekko
+import pekko.actor.ActorSystem.findClassLoader
+import pekko.actor._
+import pekko.actor.setup.ActorSystemSetup
+import pekko.util.FutureConverters.FutureOps
+
+trait ActorSystem extends org.apache.pekko.actor.ActorSystem {
+
+ /**
+ * Asynchronously terminates this actor system by running
[[CoordinatedShutdown]] with reason
+ * [[CoordinatedShutdown.ActorSystemTerminateReason]].
+ *
+ * If `pekko.coordinated-shutdown.run-by-actor-system-terminate` is
configured to `off`
+ * it will not run `CoordinatedShutdown`, but the `ActorSystem` and its
actors
+ * will still be terminated.
+ *
+ * This will stop the guardian actor, which in turn
+ * will recursively stop all its child actors, and finally the system
guardian
+ * (below which the logging actors reside) and then execute all registered
+ * termination handlers (see [[ActorSystem.registerOnTermination]]).
+ * Be careful to not schedule any operations on completion of the returned
future
+ * using the dispatcher of this actor system as it will have been shut down
before the
+ * future completes.
+ */
+ def terminateAsync(): CompletionStage[Terminated] = terminateImpl().asJava
+
+ /**
+ * Returns a [[CompletionStage]] which will be completed after the
[[ActorSystem]] has been terminated
+ * and termination hooks have been executed. If you registered any callback
with
+ * [[ActorSystem.registerOnTermination]], the returned Future from this
method will not complete
+ * until all the registered callbacks are finished. Be careful to not
schedule any operations,
+ * such as `onComplete`, on the dispatchers (`ExecutionContext`) of this
actor system as they
+ * will have been shut down before this future completes.
+ */
+ override def getWhenTerminated: CompletionStage[Terminated] =
whenTerminatedImpl.asJava
+}
+
+object ActorSystem {
+
+ /**
+ * Creates a new ActorSystem with the name "default",
+ * obtains the current ClassLoader by first inspecting the current threads'
getContextClassLoader,
+ * then tries to walk the stack to find the callers class loader, then falls
back to the ClassLoader
+ * associated with the ActorSystem class.
+ * Then it loads the default reference configuration using the ClassLoader.
+ */
+ def create(): ActorSystem = create("default")
+
+ def create(name: String): ActorSystem = create(name, None, None, None)
+
+ def create(name: String, setup: ActorSystemSetup): ActorSystem = {
+ val bootstrapSettings = setup.get[BootstrapSetup]
+ val cl =
bootstrapSettings.flatMap(_.classLoader).getOrElse(findClassLoader())
+ val appConfig =
bootstrapSettings.flatMap(_.config).getOrElse(ConfigFactory.load(cl))
+ val defaultEC = bootstrapSettings.flatMap(_.defaultExecutionContext)
+
+ val impl = new ActorSystemImpl(name, appConfig, cl, defaultEC, None,
setup) with ActorSystem {
+ // TODO: Remove in Pekko 2.0.0, not needed anymore
+ override def getWhenTerminated: CompletionStage[Terminated] =
super[ActorSystem].getWhenTerminated
Review Comment:
[nitpick] The override of getWhenTerminated is redundant since the trait
already provides this implementation. This TODO suggests it's temporary
scaffolding - consider whether this override is actually needed.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
