fix: resolves and addition tests for child preStart supervise #1385

Roiocam · 2024-07-03T01:26:10Z

Related with #1384 and resolves #1384, some of code contributed by @sadekmunawar

if PR can reproducer this issue, the ci checks will be unable to successd: https://github.com/apache/pekko/actions/runs/9770536752/job/26971887726?pr=1385

Roiocam · 2024-07-03T01:50:11Z

From the perspective of stack frame, the problem lies in the handling:

pekko/actor/src/main/scala/org/apache/pekko/actor/dungeon/FaultHandling.scala

Lines 158 to 163 in 46c5574

    
           protected def faultCreate(): Unit = { 
        
             assert(mailbox.isSuspended, "mailbox must be suspended during failed creation, status=" + mailbox.currentStatus) 
        
             assert(perpetrator == self) 
        
             cancelReceiveTimeout() 
        
             cancelReceiveTimeoutTask()


[ERROR] [07/03/2024 09:46:12.113] [SupervisorSpec-pekko.actor.default-dispatcher-5] [pekko://SupervisorSpec/user/$a/$a] changing Recreate into Create after org.apache.pekko.actor.ActorInitializationException: pekko://SupervisorSpec/user/$a/$a: exception during creation, root cause message: [deliberate test failure]
[ERROR] [07/03/2024 09:46:12.114] [SupervisorSpec-pekko.actor.default-dispatcher-6] [pekko://SupervisorSpec/user/$a/$a] assertion failed
java.lang.AssertionError: assertion failed
	at scala.Predef$.assert(Predef.scala:264)
	at org.apache.pekko.actor.dungeon.FaultHandling.faultCreate(FaultHandling.scala:160)
	at org.apache.pekko.actor.dungeon.FaultHandling.faultCreate$(FaultHandling.scala:158)
	at org.apache.pekko.actor.ActorCell.faultCreate(ActorCell.scala:420)
	at org.apache.pekko.actor.dungeon.FaultHandling.faultRecreate(FaultHandling.scala:96)
	at org.apache.pekko.actor.dungeon.FaultHandling.faultRecreate$(FaultHandling.scala:92)
	at org.apache.pekko.actor.ActorCell.faultRecreate(ActorCell.scala:420)
	at org.apache.pekko.actor.ActorCell.invokeAll$1(ActorCell.scala:526)
	at org.apache.pekko.actor.ActorCell.systemInvoke(ActorCell.scala:545)
	at org.apache.pekko.dispatch.Mailbox.processAllSystemMessages(Mailbox.scala:297)
	at org.apache.pekko.dispatch.Mailbox.run(Mailbox.scala:232)
	at org.apache.pekko.dispatch.Mailbox.exec(Mailbox.scala:245)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:175)

Roiocam · 2024-07-03T03:18:14Z

actor/src/main/scala/org/apache/pekko/actor/ActorCell.scala

@@ -643,7 +643,7 @@ private[pekko] class ActorCell(
    def failActor(): Unit =
      if (_actor != null) {
        clearActorFields(actor, recreate = false)
-        setFailedFatally()
+        setFailed(actor.self)


I am not sure if this change will break other cases or not, I will investigate later.

relate PR:

https://github.com/akka/akka/pull/29098/files

https://github.com/akka/akka/pull/27614/files

After investigation, we can not set the fault state here... otherwise, we can not distinguish between fatal and normal fault

(these links to Akka PRs are OK because they're old, pre-BSL PRs :) )

He-Pin · 2024-07-03T05:39:05Z

I think we can attach a test case for this, will look into this over the weekend.

actor-tests/src/test/scala/org/apache/pekko/actor/SupervisorSpec.scala

Roiocam · 2024-07-05T10:00:39Z

@sadekmunawar Hi, my unit test partly refers to your code in #1383 links.

Would you like to contribute this to the Pekko community?

Roiocam · 2024-07-05T10:01:51Z

Would you like to contribute this to the Pekko community?

@pjfanning Am I doing this correctly, may I ask?

pjfanning · 2024-07-05T12:12:34Z

Would you like to contribute this to the Pekko community?

@pjfanning Am I doing this correctly, may I ask?

The test code was written by https://discuss.lightbend.com/u/nodeninja/summary

@sadekmunawar can you confirm if that is you? We received your iCLA.

sadekmunawar · 2024-07-05T13:27:09Z

The test code was written by https://discuss.lightbend.com/u/nodeninja/summary

@sadekmunawar can you confirm if that is you? We received your iCLA.

@pjfanning yes, that's me. After finding the bug, I wrote that test for a solution I implemented. My solution converts the FatallyFailed into a case class and attaches an Optional ActorRef to it.

I also wrote a scala version of this test in ActorLifCyleSpec. Feel free to use either version of the test

"must not break supervisor strategy due to unhandled exception in preStart" in {
  val id = newUuid.toString
  val gen = new AtomicInteger(0)
  val maxRetryNum = 3

  val childProps = Props(new LifeCycleTestActor(testActor, id, gen) {
    override def preStart(): Unit = {
      report("preStart")
      throw new Exception("test exception")
    }
  }).withDeploy(Deploy.local)

  val supervisorStrategy: SupervisorStrategy =
    OneForOneStrategy(maxNrOfRetries = maxRetryNum, timeout.duration) {
      case _: ActorInitializationException => SupervisorStrategy.Restart
      case _                               => SupervisorStrategy.Escalate
    }
  val supervisor =  system.actorOf(Props(classOf[Supervisor], supervisorStrategy))
  Await.result((supervisor ? childProps).mapTo[ActorRef], timeout.duration)

  (0 to maxRetryNum).foreach(i => {
    expectMsg(("preStart", id, i))
  })
  expectNoMessage()
  system.stop(supervisor)
}

actor/src/main/scala/org/apache/pekko/actor/dungeon/FaultHandling.scala

pjfanning

lgtm

laglangyue · 2024-07-06T09:22:14Z

lgtm.
Although I have read the relevant code, I did not delve into testing and debugging

Roiocam · 2024-07-08T07:33:01Z

actor-tests/src/test/scala/org/apache/pekko/actor/ActorLifeCycleSpec.scala

@@ -160,6 +160,33 @@ class ActorLifeCycleSpec extends PekkoSpec with BeforeAndAfterEach with Implicit
      a ! "hello"
      expectMsg(42)
    }
+
+    "not break supervisor strategy due to unhandled exception in preStart" in {


Contributed by @sadekmunawar

Roiocam · 2024-07-08T07:34:27Z

I also wrote a scala version of this test in ActorLifCyleSpec. Feel free to use either version of the test

Thanks, I have updated this PR and verified it works in locally

pjfanning · 2024-07-15T21:54:53Z

@Roiocam should we backport this to this 1.0.x branch?

…#1385) * chore: add unit test for child actor initial exception supervise * make assertion correct * trying to fix assertion failed * distinguish between fatal and normal fault * fix unit tests * trying to fix * fix NPE * revert isFailed condition * revert isFailed place * additional tests

Roiocam · 2024-07-16T00:51:31Z

@Roiocam should we backport this to this 1.0.x branch?

Of course, this is a bug. backport on #1396

…#1396) * chore: add unit test for child actor initial exception supervise * make assertion correct * trying to fix assertion failed * distinguish between fatal and normal fault * fix unit tests * trying to fix * fix NPE * revert isFailed condition * revert isFailed place * additional tests

sadekmunawar · 2024-07-17T21:01:54Z

It looks like the checks did not run the tests in SupervisorHierarchySpec.

When I ran the tests locally, two tests in SupervisorHierarchySpec failed.

handle failure in creation when supervision strategy returns Resume and Restart
survive being stressed

Can someone check if these two unit tests pass?

pjfanning · 2024-07-17T21:16:14Z

It looks like the checks did not run the tests in SupervisorHierarchySpec.

When I ran the tests locally, two tests in SupervisorHierarchySpec failed.

handle failure in creation when supervision strategy returns Resume and Restart

survive being stressed

Can someone check if these two unit tests pass?

Those tests are tagged as long-running tests. Maybe such tests are not run by default.

Did those tests fail before this PR was added?

sadekmunawar · 2024-07-18T01:21:00Z

Did those tests fail before this PR was added?

No, they passed in my local workspace before this PR was added. Unless there's something wrong with my local setup, this PR is the likely cause of the failures. I ran into issues with these two unit tests when experimenting with my own fix.

Roiocam · 2024-07-18T05:31:15Z

I will investigate it. :)

Roiocam · 2024-07-18T07:05:37Z

both survive being stressed and handle failure in creation when supervision strategy returns Resume and Restart has passes in my local by using #1399

Roiocam added 2 commits July 3, 2024 09:25

chore: add unit test for child actor initial exception supervise

8797da0

make assertion correct

5370327

trying to fix assertion failed

5d415bd

Roiocam commented Jul 3, 2024

View reviewed changes

He-Pin reviewed Jul 3, 2024

View reviewed changes

actor-tests/src/test/scala/org/apache/pekko/actor/SupervisorSpec.scala Show resolved Hide resolved

distinguish between fatal and normal fault

e8a5787

Roiocam marked this pull request as draft July 3, 2024 06:53

Roiocam added 3 commits July 3, 2024 15:02

fix unit tests

61dedcd

trying to fix

9e324fb

fix NPE

13f9896

Roiocam marked this pull request as ready for review July 3, 2024 09:53

Roiocam added 2 commits July 3, 2024 18:19

revert isFailed condition

2d90f59

revert isFailed place

fb8d486

Roiocam requested review from jrudolph, He-Pin and raboof July 3, 2024 10:23

Roiocam changed the title ~~chore: add unit test for child actor initial exception supervise~~ fix: resolves and addition tests for child preStart supervise Jul 3, 2024

Roiocam requested a review from pjfanning July 5, 2024 10:02

pjfanning reviewed Jul 5, 2024

View reviewed changes

actor/src/main/scala/org/apache/pekko/actor/dungeon/FaultHandling.scala Show resolved Hide resolved

pjfanning approved these changes Jul 6, 2024

View reviewed changes

additional tests

42aba83

Roiocam commented Jul 8, 2024

View reviewed changes

pjfanning merged commit 73c9362 into apache:main Jul 15, 2024
18 checks passed

pjfanning added this to the 1.1.0-M2 milestone Jul 15, 2024

Roiocam deleted the child-init-exception-supervise-spec branch July 16, 2024 00:44

Roiocam mentioned this pull request Jul 18, 2024

fix: only setFailed when actor not null #1399

Merged

pjfanning mentioned this pull request Jul 18, 2024

long running tests should be run in a nightly build #1400

Closed

pjfanning added the release notes Need to release note label Aug 14, 2024

pjfanning mentioned this pull request Aug 23, 2024

fix: resolves and addition tests for child preStart supervise (#1385) #1396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolves and addition tests for child preStart supervise #1385

fix: resolves and addition tests for child preStart supervise #1385

Roiocam commented Jul 3, 2024 •

edited

Loading

Roiocam commented Jul 3, 2024 •

edited

Loading

Roiocam Jul 3, 2024

Roiocam Jul 3, 2024 •

edited

Loading

raboof Jul 3, 2024

He-Pin commented Jul 3, 2024

Roiocam commented Jul 5, 2024

Roiocam commented Jul 5, 2024

pjfanning commented Jul 5, 2024

sadekmunawar commented Jul 5, 2024 •

edited

Loading

pjfanning left a comment

laglangyue commented Jul 6, 2024

Roiocam Jul 8, 2024

Roiocam commented Jul 8, 2024

pjfanning commented Jul 15, 2024

Roiocam commented Jul 16, 2024

sadekmunawar commented Jul 17, 2024

pjfanning commented Jul 17, 2024

sadekmunawar commented Jul 18, 2024

Roiocam commented Jul 18, 2024

Roiocam commented Jul 18, 2024

fix: resolves and addition tests for child preStart supervise #1385

fix: resolves and addition tests for child preStart supervise #1385

Conversation

Roiocam commented Jul 3, 2024 • edited Loading

Roiocam commented Jul 3, 2024 • edited Loading

Roiocam Jul 3, 2024

Choose a reason for hiding this comment

Roiocam Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

raboof Jul 3, 2024

Choose a reason for hiding this comment

He-Pin commented Jul 3, 2024

Roiocam commented Jul 5, 2024

Roiocam commented Jul 5, 2024

pjfanning commented Jul 5, 2024

sadekmunawar commented Jul 5, 2024 • edited Loading

pjfanning left a comment

Choose a reason for hiding this comment

laglangyue commented Jul 6, 2024

Roiocam Jul 8, 2024

Choose a reason for hiding this comment

Roiocam commented Jul 8, 2024

pjfanning commented Jul 15, 2024

Roiocam commented Jul 16, 2024

sadekmunawar commented Jul 17, 2024

pjfanning commented Jul 17, 2024

sadekmunawar commented Jul 18, 2024

Roiocam commented Jul 18, 2024

Roiocam commented Jul 18, 2024

Roiocam commented Jul 3, 2024 •

edited

Loading

Roiocam commented Jul 3, 2024 •

edited

Loading

Roiocam Jul 3, 2024 •

edited

Loading

sadekmunawar commented Jul 5, 2024 •

edited

Loading