Skip to content

Conversation

@mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Nov 12, 2025

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 current terminate method returns a Future making it impossible to create the appropriate return type for javadsl/scaladsl (javadsl should return CompletionStage where as scaladsl should return Future). Instead we have terminateAsync which returns Future for scaladsl and CompletionStage for javadsl
  • close: A method with a configurable timeout that blocks on termination. Also implements the Java AutoCloseable interface, meaning it will execute automatically if an ActorSystem is created in a try/catch clause.
    • Adding AutoCloseable may be too much as it is a behaviour change, might be better to do this in Pekko 2.0.0
  • closeAsync: Essentially the same as old terminate/new terminateAsync but it doesn't return a Future/CompletionStagebut rather justUnit/void. This is necessary in cases where you have the root pekko.actor.ActorSystemand still need to terminate the system in a non blocking manner, typically when usingcontext` i.e.
    case TerminateMsg(Left(false)) =>
      context.system.closeAsync()
      stop()

@mdedetrich mdedetrich marked this pull request as draft November 12, 2025 07:36
@mdedetrich mdedetrich force-pushed the add-scaladsl-actor branch 4 times, most recently from febc078 to b8cbe6e Compare November 12, 2025 07:48
@mdedetrich mdedetrich force-pushed the add-scaladsl-actor branch 3 times, most recently from 3c2525a to 48aa1ac Compare November 12, 2025 08:21
import java.util.Optional
import java.util.concurrent._
import java.util.concurrent.atomic.AtomicReference

Copy link
Member

@pjfanning pjfanning Nov 12, 2025

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?

@mdedetrich mdedetrich force-pushed the add-scaladsl-actor branch 2 times, most recently from 2c086d3 to 2ed0aea Compare November 12, 2025 08:38
@pjfanning
Copy link
Member

In the PR description, would it be possible to provide a succinct description of what the close, terminate, closeAsync and terminateAsync methods do?

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Nov 12, 2025

In the PR description, would it be possible to provide a succinct description of what the close, terminate, closeAsync and terminateAsync methods do?

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)

@mdedetrich mdedetrich changed the title Split ActorSystem creation into javadsl and scaladsl DRAFT: Split ActorSystem creation into javadsl and scaladsl Nov 12, 2025
@mdedetrich
Copy link
Contributor Author

In the PR description, would it be possible to provide a succinct description of what the close, terminate, closeAsync and terminateAsync methods do?

Done

@mdedetrich mdedetrich force-pushed the add-scaladsl-actor branch 7 times, most recently from 3d7b88b to ab63a25 Compare November 12, 2025 11:23
name: String,
config: Option[Config] = None,
classLoader: Option[ClassLoader] = None,
defaultExecutionContext: Option[ExecutionContext] = None): ActorSystem =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional

Copy link
Member

@He-Pin He-Pin left a 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?

@mdedetrich
Copy link
Contributor Author

I think the shape is great, but what about the AutoCloseable?

Its implemented

@He-Pin He-Pin requested a review from Copilot November 12, 2025 11:41
@He-Pin He-Pin added this to the 1.3.0 milestone Nov 12, 2025
@He-Pin
Copy link
Member

He-Pin commented Nov 12, 2025

@mkurz FYI too

Copy link
Contributor

Copilot AI left a 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.ActorSystem and org.apache.pekko.actor.javadsl.ActorSystem traits with language-specific return types
  • Deprecated factory methods in base ActorSystem object
  • 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,
Copy link

Copilot AI Nov 12, 2025

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.

Suggested change
Duration(settings.config.getDuration("coordinated-shutdown.close-actor-system-timeout").toMillis,
Duration(settings.config.getDuration("pekko.coordinated-shutdown.close-actor-system-timeout").toMillis,

Copilot uses AI. Check for mistakes.
Comment on lines +744 to +748
* 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.
Copy link

Copilot AI Nov 12, 2025

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.

Suggested change
* 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.

Copilot uses AI. Check for mistakes.
* are completely on your own in that case!
*/
abstract class ActorSystem extends ActorRefFactory with ClassicActorSystemProvider {
trait ActorSystem extends ActorRefFactory with ClassicActorSystemProvider with AutoCloseable {
Copy link

Copilot AI Nov 12, 2025

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.

Suggested change
trait ActorSystem extends ActorRefFactory with ClassicActorSystemProvider with AutoCloseable {
trait ActorSystem extends ActorRefFactory with ClassicActorSystemProvider {

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +85
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
}
Copy link

Copilot AI Nov 12, 2025

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.

Copilot uses AI. Check for mistakes.
Comment on lines +79 to +82
// TODO: Remove in Pekko 2.0.0, not needed anymore
override def getWhenTerminated: CompletionStage[Terminated] = super[ActorSystem].getWhenTerminated
Copy link

Copilot AI Nov 12, 2025

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.

Suggested change
// TODO: Remove in Pekko 2.0.0, not needed anymore
override def getWhenTerminated: CompletionStage[Terminated] = super[ActorSystem].getWhenTerminated

Copilot uses AI. Check for mistakes.
*/
def terminate(): Future[Terminated]
@deprecated(
"Use .close() or the .terminateAsync() function on an ActorSystem created by org.apache.pekko.actor.scaladsl.ActorSystem.apply",
Copy link

Copilot AI Nov 12, 2025

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.

Suggested change
"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.",

Copilot uses AI. Check for mistakes.
@pjfanning
Copy link
Member

pjfanning commented Nov 12, 2025

Whatever we come up, we should make sure that the typed ActorSystem is then set up to have an equivalent API.
Same split into Java DSL / Scala DSL. Same method names and behaviours for the close* and terminate* methods. Autocloseable interface.
That can be done separately but we shouldn't release 1.3.0 or 2.0.0-M1 with changes in classic ActorSystem and no changes in typed ActorSystem.

@mdedetrich
Copy link
Contributor Author

That can be done separately but we shouldn't release 1.3.0 or 2.0.0-M1 with changes in classic ActorSystem and no changes in typed ActorSystem.

I also plan to do typed ActorSystem but that will come later

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

@He-Pin He-Pin removed this from the 1.3.0 milestone Nov 12, 2025
@He-Pin
Copy link
Member

He-Pin commented Nov 12, 2025

@mdedetrich Is there an ETA, maybe we can do that in one PR. to make sure it consistentcy

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Nov 12, 2025

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 scaladsl to work by having the actor.scaladsl.ActorSystem/actor.javadsl.ActorSystembe a trait, converting actor.ActorSystem to a trait but the same doesn't work because we need a constructor for actor.ActorSystem (some parts of Akka can instantiate a actor.ActorSystem manually by reflection, this also breaks MiMa).

I tried making ActorSystemImpl a trait to get around this, but we then get a cyclic inheritance issue. Ill have another try at it, but it looks like that in order to do this it would have to be with breaking changes in Pekko 2.0.0 where some slight adjustments to the inheritance need to be made

@mdedetrich
Copy link
Contributor Author

@mdedetrich Is there an ETA, maybe we can do that in one PR. to make sure it consistentcy

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)

@He-Pin
Copy link
Member

He-Pin commented Nov 12, 2025

then what about we delay this in 2.0.0, and let 1.3.x released soon?

@pjfanning pjfanning marked this pull request as ready for review November 12, 2025 14:22
@He-Pin He-Pin added this to the 2.0.0-M2 milestone Nov 12, 2025
@pjfanning
Copy link
Member

Thanks @mdedetrich, can we retarget at main branch and continue there?

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Nov 12, 2025

Thanks @mdedetrich, can we retarget at main branch and continue there?

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

@pjfanning
Copy link
Member

pjfanning commented Nov 12, 2025

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.
I would still prefer to see this in main branch first and then something that implements something equivalent can be backported.
In the end of day, there isn't a big demand for this. I'd prefer to proceed with 1.3.0 without this and I expect we'll have a 1.4.0 at some stage.

@mdedetrich
Copy link
Contributor Author

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 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 pekko.actor.ActorSystem (or one of its subclasses) and so delegation won't work here.

Regarding AutoCloseable, I made a PR at #2486. The AutoCloseable interface is not reliant on scala/java dsl in any way so that can be implemented irrespective of #2093

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants