-
Notifications
You must be signed in to change notification settings - Fork 186
DRAFT: Split ActorSystem creation into javadsl and scaladsl #2479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 1.3.x
Are you sure you want to change the base?
Conversation
febc078 to
b8cbe6e
Compare
3c2525a to
48aa1ac
Compare
actor/src/main/scala/org/apache/pekko/actor/scaladsl/ActorSystem.scala
Outdated
Show resolved
Hide resolved
actor/src/main/scala/org/apache/pekko/actor/javadsl/ActorSystem.scala
Outdated
Show resolved
Hide resolved
| import java.util.Optional | ||
| import java.util.concurrent._ | ||
| import java.util.concurrent.atomic.AtomicReference | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep the whitespace separating different groups of imports?
2c086d3 to
2ed0aea
Compare
|
In the PR description, would it be possible to provide a succinct description of what the close, terminate, closeAsync and terminateAsync methods do? |
2ed0aea to
50f5eb2
Compare
Will do, right now this is more of an exploratory PR to see if we should go ahead with it at all (see https://lists.apache.org/thread/0lzlhh4ll08t1o1gfh7w08635c5t6bw3) |
Done |
3d7b88b to
ab63a25
Compare
| name: String, | ||
| config: Option[Config] = None, | ||
| classLoader: Option[ClassLoader] = None, | ||
| defaultExecutionContext: Option[ExecutionContext] = None): ActorSystem = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional
He-Pin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the shape is great, but what about the AutoCloseable?
Its implemented |
ab63a25 to
96ea64b
Compare
|
@mkurz FYI too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR splits ActorSystem creation into separate javadsl and scaladsl packages to provide language-specific APIs. It introduces new termination methods (terminateAsync(), close(), closeAsync()) and deprecates the existing terminate() and factory methods in the base ActorSystem to guide users toward the language-specific APIs.
Key changes:
- New
org.apache.pekko.actor.scaladsl.ActorSystemandorg.apache.pekko.actor.javadsl.ActorSystemtraits with language-specific return types - Deprecated factory methods in base
ActorSystemobject - New termination methods with clearer semantics and AutoCloseable support
Reviewed Changes
Copilot reviewed 181 out of 181 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| actor/src/main/scala/org/apache/pekko/actor/scaladsl/ActorSystem.scala | New Scala DSL ActorSystem trait with Future-based termination methods |
| actor/src/main/scala/org/apache/pekko/actor/javadsl/ActorSystem.scala | New Java DSL ActorSystem trait with CompletionStage-based termination methods |
| actor/src/main/scala/org/apache/pekko/actor/ActorSystem.scala | Deprecated existing factory methods, added close() and closeAsync(), made trait extend AutoCloseable |
| actor/src/main/resources/reference.conf | Added configuration for close-actor-system-timeout |
| Test files (100+ files) | Updated imports and method calls to use new language-specific APIs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| override def close(): Unit = { | ||
| terminateImpl() | ||
| Await.ready(whenTerminatedImpl, | ||
| Duration(settings.config.getDuration("coordinated-shutdown.close-actor-system-timeout").toMillis, |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Duration(settings.config.getDuration("coordinated-shutdown.close-actor-system-timeout").toMillis, | |
| Duration(settings.config.getDuration("pekko.coordinated-shutdown.close-actor-system-timeout").toMillis, |
| * 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. |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for closeAsync() incorrectly states 'This method will block', but this method is non-blocking and returns Unit immediately without waiting for termination.
| * 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. | |
| * 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. |
| * are completely on your own in that case! | ||
| */ | ||
| abstract class ActorSystem extends ActorRefFactory with ClassicActorSystemProvider { | ||
| trait ActorSystem extends ActorRefFactory with ClassicActorSystemProvider with AutoCloseable { |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| trait ActorSystem extends ActorRefFactory with ClassicActorSystemProvider with AutoCloseable { | |
| trait ActorSystem extends ActorRefFactory with ClassicActorSystemProvider { |
| 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 | ||
| } |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
| // TODO: Remove in Pekko 2.0.0, not needed anymore | ||
| override def getWhenTerminated: CompletionStage[Terminated] = super[ActorSystem].getWhenTerminated |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
| // TODO: Remove in Pekko 2.0.0, not needed anymore | |
| override def getWhenTerminated: CompletionStage[Terminated] = super[ActorSystem].getWhenTerminated |
| */ | ||
| def terminate(): Future[Terminated] | ||
| @deprecated( | ||
| "Use .close() or the .terminateAsync() function on an ActorSystem created by org.apache.pekko.actor.scaladsl.ActorSystem.apply", |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
| "Use .close() or the .terminateAsync() function on an ActorSystem created by org.apache.pekko.actor.scaladsl.ActorSystem.apply", | |
| "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.", |
|
Whatever we come up, we should make sure that the typed ActorSystem is then set up to have an equivalent API. |
I also plan to do typed Also can we please not do copilot AI review until PR is done, its just going to create a whole lot of noise at a time when its not useful |
|
@mdedetrich Is there an ETA, maybe we can do that in one PR. to make sure it consistentcy |
|
So I have some bad/annoying news, it appears that its not possible to do this in Pekko 1.3.x because of how the inheritance between the various Actor classes is done and the fact that Java interfaces cannot extend abstract classes. I can get the I tried making |
There is no ETA as of yet, this is a proof of concept to see if its even possible (and given the previous comment it doesn't look like it is) |
|
then what about we delay this in 2.0.0, and let 1.3.x released soon? |
|
Thanks @mdedetrich, can we retarget at main branch and continue there? |
96ea64b to
6c446b3
Compare
Give me a couple of days to figure out, if that doesn't work then I will make a new PR against 2.0.0 as it will be slightly different and will have a better/cleaner API. Also if anyone has any other alternatives feel free to checkout the branch and play with it locally, may have forgetten something |
|
One possible 1.x solution would to leave the existing ActorSystem class as is except maybe to deprecate its methods. You can add new Scala and Java DSL classes that delegate to the existing class but that implement their own APIs and Autocloseable. The new classes would not subclass the existing class. |
The issue is that even doing this doesn't work at least without a huge amount of code churn. Direct inheritance doesn't work (as described) and delegation also won't work for other reasons, a lot of Actor code expects to work with Regarding |
This PR is an exploratory PR to see whether its worth it to resolve #2093 as a non source breaking change when going from Pekko 1.3.0 to 2.0.0. If we decide that a source breaking change in Pekko 2.0.0 is acceptable then this PR is not necessary
Explanation of methods
terminate: This one is deprecated and is no longer meant to be used. I was forced to do this since the currentterminatemethod returns aFuturemaking it impossible to create the appropriate return type forjavadsl/scaladsl(javadslshould returnCompletionStagewhere asscaladslshould returnFuture). Instead we haveterminateAsyncwhich returnsFutureforscaladslandCompletionStageforjavadslclose: A method with a configurable timeout that blocks on termination. Also implements the JavaAutoCloseableinterface, meaning it will execute automatically if anActorSystemis created in a try/catch clause.AutoCloseablemay be too much as it is a behaviour change, might be better to do this in Pekko 2.0.0closeAsync: Essentially the same as oldterminate/newterminateAsyncbut it doesn't return aFuture/CompletionStagebut rather justUnit/void. This is necessary in cases where you have the rootpekko.actor.ActorSystemand still need to terminate the system in a non blocking manner, typically when usingcontext` i.e.