Add close on ActorSystem

[mdedetrich]

Adds the close method on ActorSystem (classic and typed) as well as implementing the AutoCloseable interface, work taken from #2479. Regardless of what happens with #2093, the changes in this PR are always valid (close is blocking and returns Unit/Void so its valid for both javadsl and scaladsl).

The PR also cleans up test code by removing the Await.result(system.close,...) pattern that is used everywhere.

An overloaded close(duration: Duration) method can be added in Pekko 2.0.0 if #2479 works out, as the scaladsl will use the Scala Duration and the javadsl will use the Java Duration.

[raboof]

close is blocking and returns Unit/Void

Is there a requirement for close to block until the closing is 'complete'?

The PR also cleans up test code by removing the Await.result(system.close,...) pattern that is used everywhere.

I can see the appeal, though blocking is a bit 'uncharacteristic' for us. I guess because you're stopping the ActorSystem and thus are anyway 'on your way out' it might be fine?

[mdedetrich]

Is there a requirement for close to block until the closing is 'complete'?

I believe so otherwise you can get weird behaviour but I will do research to confirm. From what I gather (and also when learning Kotlin and using stuff like Arrow Resource), AutoCloseable is kind of pointless if the close method is asynchronous, which is why you have stuff like https://docs.datastax.com/en/latest-java-driver-api/com/datastax/oss/driver/api/core/AsyncAutoCloseable.html

I can see the appeal, though blocking is a bit 'uncharacteristic' for us. I guess because you're stopping the ActorSystem and thus are anyway 'on your way out' it might be fine?

To clarify, if I replace Await.result with close its because the behaviour of blocking is exactly the same. In those cases, the only thing that would have changed would be the timeout (i.e. sometimes it was 15 seconds, sometimes 20 but these are all tests so it shouldn't be an issue anyways).

[raboof]

Is there a requirement for close to block until the closing is 'complete'?

I believe so otherwise you can get weird behaviour but I will do research to confirm. From what I gather (and also when learning Kotlin and using stuff like Arrow Resource), AutoCloseable is kind of pointless if the close method is asynchronous

Why? When I want to make sure the cleanup is triggered but don't care too much when it happens, it might be fine?

which is why you have stuff like https://docs.datastax.com/en/latest-java-driver-api/com/datastax/oss/driver/api/core/AsyncAutoCloseable.html

It's an interesting data point that the close of that one is also blocking.

I can see the appeal, though blocking is a bit 'uncharacteristic' for us. I guess because you're stopping the ActorSystem and thus are anyway 'on your way out' it might be fine?

To clarify, if I replace Await.result with close its because the behaviour of blocking is exactly the same. In those cases, the only thing that would have changed would be the timeout (i.e. sometimes it was 15 seconds, sometimes 20 but these are all tests so it shouldn't be an issue anyways).

Sure, I meant blocking in the 'production' code is a bit uncharacteristic - in tests it's indeed definitely fine.

[mdedetrich]

Why? When I want to make sure the cleanup is triggered but don't care too much when it happens, it might be fine?

From what I recall, if there is an error in cleaning up on close() its expected that an exception be thrown which is not possible if its asynchronous but I will look into it.

I would say personally that having close() cleaning up resources in an asynchronous manner would be surprising behaviour

[mdedetrich]

So regarding blocking on close(), the official documentation from https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html i.e.

See the Javadoc of the AutoCloseable and Closeable interfaces for a list of classes that implement either of these interfaces. The Closeable interface extends the AutoCloseable interface. The close method of the Closeable interface throws exceptions of type IOException while the close method of the AutoCloseable interface throws exceptions of type Exception. Consequently, subclasses of the AutoCloseable interface can override this behavior of the close method to throw specialized exceptions, such as IOException, or no exception at all.

And more directly from https://docs.oracle.com/javase/8/docs/api/java/lang/AutoCloseable.html

While this interface method is declared to throw Exception, implementers are strongly encouraged to declare concrete implementations of the close method to throw more specific exceptions, or to throw no exception at all if the close operation cannot fail.

Cases where the close operation may fail require careful attention by implementers. It is strongly advised to relinquish the underlying resources and to internally mark the resource as closed, prior to throwing the exception. The close method is unlikely to be invoked more than once and so this ensures that the resources are released in a timely manner. Furthermore it reduces problems that could arise when the resource wraps, or is wrapped, by another resource.

Implies that it is expected that if the close() method fails that an exception should be thrown. It is possible for ActorSystem.terminate() to fail and the only way to throw an exception in that case is to block on it.

Personally I was also apprehensive of implementing close() for this exact reason, blocking isn't entirely desirable but to me it seems expected that close() methods throw an exception if the closing of the resource fails.

[pjfanning]

Would it be acceptable to not put this on ActorSystem but create a factory method like ActorSystem.createWithBlockingClose() or ActorSystem.apply(withBlockingClose: Boolean = false)? The returned ActorSystem would mix in a close method.

We could also support mixing in a non-blocking close.

[mdedetrich]

Would it be acceptable to not put this on ActorSystem but create a factory method like ActorSystem.createWithBlockingClose() or ActorSystem.apply(withBlockingClose: Boolean = false)?

Well I would still have to put it on ActorSystem otherwise AutoCloseable won't work, but I can make the blocking behaviour configurable (alternately it can go in reference.conf though given that close should block by default)

I think the point is, if you are going to use close() and don't want it to block, at that point you are already rolling your own resource cleaning mechanism and so close() + AutoCloseable is kind of pointless. There are also already methods that do close in an async manner (thats what .terminate() does).

This really does seem to be one of those cases where either we implement close as blocking by default, or we don't implement it at all as to "force"/encourage users to not block on ActorSystem termination. I still fall into the latter camp, but I do admit I haven't seen a case where a blocking ActorSystem.close() could cause issues in the common case (it can create deadlocks if you have blocking code in actors, which you shouldn't, but thats why the timeout is there).

[pjfanning]

Would it be acceptable to not put this on ActorSystem but create a factory method like ActorSystem.createWithBlockingClose() or ActorSystem.apply(withBlockingClose: Boolean = false)?

Well I would still have to put it on ActorSystem otherwise AutoCloseable won't work, but I can make the blocking behaviour configurable (alternately it can go in reference.conf though given that close should block by default)>

I think the point is, if you are going to use close() and don't want it to block, at that point you are already rolling your own resource cleaning mechanism at which point close() + AutoCloseable is kind of pointless. There are also already methods that do close in an async manner (thats what .terminate() does).

A config option to decide on whether the close is blocking alongside a config to control the timeout for blocking seems ok.

[mdedetrich]

A config option to decide on whether the close is blocking alongside a config to control the timeout for blocking seems ok.

Cool, Ill see feedback from others as well

[He-Pin]

Once get merged, this should be ported to 2.0.0

btw, @mdedetrich what's the status of kotlin dsl of Pekko?

[He-Pin]

add @since 1.3.0

[mdedetrich]

Same as #2486 (comment), this is not part of public API

[He-Pin]

add since 1.3.0

[mdedetrich]

Do we have an example of what this is meant to look like?

[mdedetrich]

btw, @mdedetrich what's the status of kotlin dsl of Pekko?

I have some good news on this front, there is an official sbt kotlin plugin by jetbrains so it should be largely trivial https://github.com/JetBrains/sbt-kotlin-plugin

The thing is that its blocked by #2093 as kotlion also has its own way of handling async IO (it has suspend functions)

[mdedetrich]

I will wait a few days to see if @raboof responds and also his views on making the blocking nature configurable

[mdedetrich]

I have just updated the PR to actually add tests to make sure the AutoCloseable works as expected. There is a Java test that uses try-with-resources and there is a Scala test using scala.util.Using (which automatically uses AutoCloseable).

One note is that when I cherry pick this into Pekko 1.3.x, the scala.util.Using tests will only work for Scala 2.13/Scala 3 so I will need to modify the tests there.

[pjfanning]

lgtm

[He-Pin]

@mdedetrich feel free to merge this and send a backport to 1.3.x

[He-Pin]

I will do the backporting

[mdedetrich]

I will do the backporting

Please let me do it as I have to modify a test

[He-Pin]

@mdedetrich Nice you are online, hands over~