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

Akka Typed watchWith failures #25803

Closed
huntc opened this Issue Oct 17, 2018 · 14 comments

Comments

Projects
3 participants
@huntc
Copy link
Contributor

huntc commented Oct 17, 2018

When using watchWith in Akka Typed, I'm unable to discern between what has caused the termination e.g. whether the actor stopped itself or threw an exception. All that I know is that the watched actor has terminated.

I had assumed (for some reason) that watchWith might still cause Terminated to be sent to the observing actor in the case of a failure. This doesn't appear to be the case though.

Should I revert to just using watch in cases where I need to discern the cause of termination, or might it be useful to register an additional message with watchWith to fire when there is abnormal termination... or even just have watchWith send Terminated in such an instance?

As a slight aside, I find it annoying that Akka logs the abnormal termination as an error... Is there a means to avoid logging in such a circumstance?

huntc added a commit to huntc/alpakka that referenced this issue Oct 17, 2018

Client termination notifications
The caller may now source a stream of client termination events so that they can perform any requisite cleanup.

One test is failing due to my confusion of failing actors as documented here: akka/akka#25803

@huntc huntc referenced this issue Oct 17, 2018

Merged

MQTT Streaming #1225

@patriknw

This comment has been minimized.

Copy link
Member

patriknw commented Oct 17, 2018

The primary purpose of watchWith is to translate the termination signal into the message protocol of the watching actors (without using signals).

Should I revert to just using watch in cases where I need to discern the cause of termination, or might it be useful to register an additional message with watchWith to fire when there is abnormal termination... or even just have watchWith send Terminated in such an instance?

I think it would be easiest if you use watch for that case. Other alternatives seems to make the API rather much more complicated.

As a slight aside, I find it annoying that Akka logs the abnormal termination as an error... Is there a means to avoid logging in such a circumstance?

That logging can be disabled via the supervision strategy.

Behaviors.supervise(targetBehavior)
  .onFailure[Throwable](SupervisorStrategy.stop.withLoggingEnabled(false))
@huntc

This comment has been minimized.

Copy link
Contributor

huntc commented Oct 17, 2018

In summary, even when using watch, I'm unable to discern between a child actor stopping gracefully vs stopping given a failure.

On using watch instead, the downside for me is that the normal handling of an actor termination is out of band to my general receive handling. It makes the code slightly harder to follow, but not hugely so I guess.

I've now changed my code to use only watch but I still don't see the failure field being set when I know that the child actor has stopped because of failure. Any ideas? Here's the relevant line where I'm not seeing a failure field value: https://github.com/akka/alpakka/pull/1225/files#diff-bb3dfa9f485134e7d00a1749fe1de18dR182. I have observed that an exception is causing the child actor to stop.

Thanks for the logging advice.

@patriknw

This comment has been minimized.

Copy link
Member

patriknw commented Oct 18, 2018

In summary, even when using watch, I'm unable to discern between a child actor stopping gracefully vs stopping given a failure.

I've now changed my code to use only watch but I still don't see the failure field being set when I know that the child actor has stopped because of failure. Any ideas? Here's the relevant line where I'm not seeing a failure field value

That information should be included via the failure: Option[Throwable] in the Terminated.
We have a test for that in https://github.com/akka/akka/blob/master/akka-actor-typed-tests/src/test/scala/akka/actor/typed/WatchSpec.scala#L108 so it's strange that you don't see it in your code.

@huntc

This comment has been minimized.

Copy link
Contributor

huntc commented Oct 18, 2018

Thanks for the pointer to the test. I'm not quite sure what to test next... I shall of course look further, but if you have any further inspiration then it would be welcome. Thanks.

@huntc

This comment has been minimized.

Copy link
Contributor

huntc commented Oct 18, 2018

@patriknw The problem appears to be to do with when declaring a supervisor strategy. If I change your test to:

        val child = ctx.spawn(Behaviors.supervise(Behaviors.receive[Any]((ctx, msg) 
          throw ex
        )).onFailure(SupervisorStrategy.stop), "child")

...then it fails. Accordingly, if I remove the supervisor declaration from my code then it works. :-)

@patriknw

This comment has been minimized.

Copy link
Member

patriknw commented Oct 18, 2018

That's a good finding. It's a bug.

@huntc

This comment has been minimized.

Copy link
Contributor

huntc commented Oct 18, 2018

Good to know - I'll let it log the errors for now - at least I don't get horrid stack traces!

I tried to figure out how the code worked re. supervision so I could raise a PR, but I guess it may just be beyond me. :-)

@patriknw patriknw added this to Backlog in Akka Typed Oct 18, 2018

huntc added a commit to huntc/alpakka that referenced this issue Oct 19, 2018

Supervision loses the failure field
As per the bug described at akka/akka#25803 (comment), I’ve now removed supervision and will just put up with the logging. We could easily revert this commit at a later stage if required i.e. when the bug is fixed, and if we still deem the logging to be a real problem.
@patriknw

This comment has been minimized.

Copy link
Member

patriknw commented Nov 5, 2018

When I thought some more about it afterwards I'm skeptical to this exception passing via watch because sending exceptions in remote messages is difficult (serialization problems). Just having them for watch of local child actors doesn't align well with location transparency principles.

@huntc

This comment has been minimized.

Copy link
Contributor

huntc commented Nov 5, 2018

I don’t know what else other than an exception to convey abnormal termination to an observer.

Also, to revisit, my main requirement is to discern between an actor terminating normally and abnormally given some exceptional, but recoverable, circumstance.

@chbatey

This comment has been minimized.

Copy link
Member

chbatey commented Nov 27, 2018

As it stands the Typed supervision mechanism stops the exception reaching the ActorAdapter. The exception is only filled in if the adapter's supervision gets the exception.

I think for now let's remove the exception from Terminated as it is broken by typed supervision and it only was designed to work for direct children.

@huntc

This comment has been minimized.

Copy link
Contributor

huntc commented Nov 27, 2018

I need to discern between a direct child terminating abnormally vs not. How would you propose that I do that if you remove the exception from Terminated?

Please note also that the Alpakka MQTT streaming connector relies on the ability to make this distinction.

@chbatey

This comment has been minimized.

Copy link
Member

chbatey commented Nov 29, 2018

@huntc looking at the Alpakka MQTT connector and the use case of bubbling up exceptions I agree we should keep it. We'll make it clean with a specific ChildTerminated rather than an option in the normal Terminated message. Typed won't support remote deployment so children are always local.

@huntc

This comment has been minimized.

Copy link
Contributor

huntc commented Nov 29, 2018

That sounds great! Thanks for the consideration.

@patriknw patriknw moved this from In Progress to Reviewing in Akka Typed Nov 30, 2018

@patriknw patriknw added this to the 2.5.19 milestone Dec 7, 2018

@patriknw

This comment has been minimized.

Copy link
Member

patriknw commented Dec 7, 2018

@huntc This was fixed in #25992 by introducing a new ChildTerminated signal. Created Alpakka issue akka/alpakka#1359

@patriknw patriknw closed this Dec 7, 2018

Akka Typed automation moved this from Reviewing to Done Dec 7, 2018

@patriknw patriknw referenced this issue Dec 7, 2018

Merged

Akka 2.5.19 #534

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