You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@pekko.apache.org by "He-Pin (via GitHub)" <gi...@apache.org> on 2026/10/13 15:44:34 UTC

[PR] Revert unconditional fork-join task wrapping from #3621 [pekko]

He-Pin opened a new pull request, #3628:
URL: https://github.com/apache/pekko/pull/3628

### Motivation

#3621 made `Mailbox` a plain `Runnable`, so the existing executor implementation already creates a fresh task wrapper for each mailbox submission. Unconditionally wrapping an existing `ForkJoinTask` extends the change beyond mailboxes and can bypass its cancellation state by calling its `Runnable.run()` directly.

### Modification

- Restore `PekkoForkJoinPool.execute(Runnable)` exactly to its implementation before #3621.
- Preserve the `Mailbox` inheritance fix, its MiMa filters, and the existing `PekkoForkJoinTask` interruption handling.
- Replace the expectation that a failed `ForkJoinTask` can be rerun without reinitialization with a regression assertion that a cancelled task submitted as `Runnable` is not executed.
- Verify in the regression suite that a plain reused `Runnable` can execute again after a failing run, waiting for quiescence before resubmission.

### Result

Mailbox submissions continue to receive fresh task identities. Existing fork-join tasks retain their own execution state; their callers remain responsible for safe reuse and reinitialization after completion. The override retains Pekko's handling of `InterruptedException`, which restores the interrupt flag without invoking the uncaught-exception handler.

### Tests

- Not run locally at the contributor's request; runtime validation is deferred to remote CI. The revised regression tests have not been executed locally.
- Native `scalafmt --mode diff-ref=origin/main` and `git diff --check` passed.
- Confirmed that `ForkJoinExecutorConfigurator.scala` matches the pre-#3621 file exactly and that `Mailbox.scala` and the MiMa filters are unchanged.
- Auditable Qoder stdout review: No must-fix findings.

### References

Refs #3621.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org

Re: [PR] Revert unconditional fork-join task wrapping from #3621 [pekko]

Posted by "He-Pin (via GitHub)" <gi...@apache.org>.

He-Pin merged PR #3628:
URL: https://github.com/apache/pekko/pull/3628


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org

Re: [PR] Revert unconditional fork-join task wrapping from #3621 [pekko]

Posted by "pjfanning (via GitHub)" <gi...@apache.org>.

pjfanning commented on code in PR #3628:
URL: https://github.com/apache/pekko/pull/3628#discussion_r4268693891


##########
actor/src/main/scala/org/apache/pekko/dispatch/ForkJoinExecutorConfigurator.scala:
##########
@@ -60,11 +60,8 @@ object ForkJoinExecutorConfigurator {
 
     override def execute(r: Runnable): Unit =
       if (r ne null)
-        // Always hand the pool a task object it has not seen before. Pushing a caller-owned
-        // ForkJoinTask lets the same reference return to the same work-queue slot, which a steal
-        // that stalls between its base recheck and its slot CAS turns into a permanently damaged
-        // queue: https://github.com/ben-manes/caffeine/issues/2021
-        super.execute(new PekkoForkJoinTask(r))
+        super.execute(
+          (if (r.isInstanceOf[ForkJoinTask[?]]) r else new PekkoForkJoinTask(r)).asInstanceOf[ForkJoinTask[Any]])

Review Comment:
   Non-blocking: a short comment here would stop someone from "fixing" the pass-through again later.
   
   ```suggestion
           // An existing ForkJoinTask is pushed as-is (matching ForkJoinPool.execute) so that its own
           // completion and cancellation state is honoured. Callers must not resubmit the same task
           // instance (see https://github.com/ben-manes/caffeine/issues/2021); this is why Mailbox
           // is deliberately not a ForkJoinTask.
           super.execute(
             (if (r.isInstanceOf[ForkJoinTask[?]]) r else new PekkoForkJoinTask(r)).asInstanceOf[ForkJoinTask[Any]])
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org

Re: [PR] Revert unconditional fork-join task wrapping from #3621 [pekko]

Posted by "pjfanning (via GitHub)" <gi...@apache.org>.

pjfanning commented on PR #3628:
URL: https://github.com/apache/pekko/pull/3628#issuecomment-6156376547

LGTM overall. Reverting the unconditional wrap is right: with `Mailbox` no longer a `ForkJoinTask` (and `TaskInvocation` never one), no Pekko main code reaches the pass-through branch, so this doesn't reintroduce caffeine#2021 / #3620. The restored code also matches the JDK's own `ForkJoinPool.execute(Runnable)`, which pushes a `ForkJoinTask` as-is and wraps anything else.

The motivation is arguably stronger than cancellation alone: wrapping a caller's `ForkJoinTask` means the task itself is never completed, so `pool.execute(t: Runnable); t.join()` hangs forever when `t.run()` doesn't route through `invoke()` (JDK adapters do; a custom `ForkJoinTask with Runnable` typically doesn't). Might be worth adding to Motivation.

A few small comments:

1. **Keep a comment on the restored branch** (`ForkJoinExecutorConfigurator.scala`). The #3621 comment is removed and nothing replaces it, so this is easy to "fix" again later. Something like:
```scala
// Existing ForkJoinTasks are pushed as-is (matching ForkJoinPool.execute) so their own
// completion/cancellation state is honoured. Callers must not resubmit the same task
// instance (see caffeine#2021); Mailbox is deliberately not a ForkJoinTask for that reason.
```
2. **Optional extra test:** asserting that `task.join()` returns (or `task.isDone` becomes true) after `pool.execute(task: Runnable)` would cover the completion-state regression, which hangs with the #3621 code.
3. **Assertion in `finally`:** in the cancelled-task test, `awaitTermination(...) shouldBe true` inside `finally` can mask the original exception if the `try` body fails. Consider moving the assertion after the `try`.
4. **Plain-`Runnable` test:** it passes both before and after #3621 (each submission gets a fresh wrapper), so it's a control rather than a regression test. The `awaitAssert(pool.isQuiescent shouldBe true)` isn't needed for that, since the two submissions are different wrapper objects; either drop it or add a comment saying why it's there.
5. **PR body wording:** "The override retains Pekko's handling of `InterruptedException`" applies only to runnables that aren't `ForkJoinTask`s; tasks passed through use their own `exec()`. And the "Tests: not run locally" note can now point at the green CI runs.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org