fix: deliver stashed timer messages for UnboundedStash and UnrestrictedStash (#3258)#3264
Open
He-Pin wants to merge 1 commit into
Open
fix: deliver stashed timer messages for UnboundedStash and UnrestrictedStash (#3258)#3264He-Pin wants to merge 1 commit into
He-Pin wants to merge 1 commit into
Conversation
…edStash Motivation: Timers rewrites currentMessage to the unwrapped timer message before invoking the user's receive, so that stash() captures the actual message rather than the internal TimerMsg wrapper. This rewrite was guarded by isInstanceOf[Stash], which is false for actors mixing in UnboundedStash or UnrestrictedStash directly - they are siblings of Stash (all extend UnrestrictedStash), not subtypes. As a result stash() captured the TimerMsg wrapper, and on unstashAll() the wrapper was intercepted again and discarded (single-shot timer already removed, or stale generation for a periodic timer), losing the message. Modification: - Guard the currentMessage rewrite with isInstanceOf[StashSupport] instead of isInstanceOf[Stash], so all three public stash traits (Stash, UnboundedStash, UnrestrictedStash - which all extend StashSupport) receive the unwrapped message. Result: Timer messages stashed with UnboundedStash or UnrestrictedStash are no longer lost on unstash; behavior for Stash is unchanged and non-stashing actors are unaffected. Tests: - actor-tests/testOnly org.apache.pekko.actor.TimerSpec org.apache.pekko.actor.TimersAndStashSpec - all passed. Added directional tests for UnboundedStash and UnrestrictedStash that fail before the fix (the timer message is lost on unstash). - actor/mimaReportBinaryIssues - no binary issues. References: Fixes #3258
He-Pin
commented
Jun 28, 2026
| case OptionVal.Some(m) => | ||
| if (this.isInstanceOf[Stash]) { | ||
| // this is important for stash interaction, as stash will look directly at currentMessage #24557 | ||
| if (this.isInstanceOf[StashSupport]) { |
Member
Author
There was a problem hiding this comment.
Member
Author
|
@pjfanning I think we can backport this to 1.7.0 ,wdyt |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
TimersrewritesactorCell.currentMessageto the unwrapped timer messagebefore invoking the user's
receive, so thatstash()(which readscurrentMessagedirectly) captures the actual message rather than the internalTimerMsgwrapper.That rewrite was guarded by
this.isInstanceOf[Stash]. ButStash,UnboundedStashandUnrestrictedStashare siblings — they all extendUnrestrictedStash/StashSupport, andUnboundedStash/UnrestrictedStashare not subtypes of
Stash. So for an actor mixing inUnboundedStashorUnrestrictedStashdirectly, the guard wasfalse, the rewrite was skipped,and
stash()captured theTimerMsgwrapper. OnunstashAll()the wrapper wasintercepted again and discarded (the single-shot timer was already removed, or
the generation was stale for a periodic timer), so the message was lost.
The same guard also excluded the Java‑API stash actors (
AbstractActorWithStashand friends, via
AbstractActorStashSupport), which have the same problem.Fixes #3258.
Modification
currentMessagerewrite withthis.isInstanceOf[StashSupport]instead of
this.isInstanceOf[Stash].StashSupportis the common base ofall stash‑capable actors (
Stash,UnboundedStash,UnrestrictedStash, andthe Java‑API stash classes), and it is exactly the set of actors whose
stash()readscurrentMessage— so it is the precise, correct boundary.re‑stash/lost‑message mechanism are clear (keeping the original
#24557reference).
Result
UnboundedStashorUnrestrictedStash(and theJava‑API stash actors) are no longer lost on
unstashAll().Stashis unchanged, and non‑stashing actors are unaffected(the guard is still
falsefor them).Timers.aroundReceive; no public APIor binary‑compatibility change (
actor / mimaReportBinaryIssuesis clean).Tests
sbt "actor-tests/testOnly org.apache.pekko.actor.TimerSpec org.apache.pekko.actor.TimersAndStashSpec"→ all passed.
Stashtest, forUnboundedStashandUnrestrictedStash(the latter with an explicitdeque‑based mailbox, since
UnrestrictedStashdoes not declareRequiresMessageQueue). Both fail before the fix (the stashed timermessage is lost:
expectMsg("scheduled")times out) and pass after.sbt "actor/mimaReportBinaryIssues"→ no binary issues.References
Fixes #3258
This is an original contribution to Apache Pekko, made under the Apache License
2.0 (i.e. the changes are now Apache licensed). No third-party or Akka-derived
code is included.