Skip to content

Conversation

@thomasjball
Copy link

@thomasjball thomasjball commented Nov 21, 2025

this changes fixes a concurrency bug that can happen when a listener is ignored, then the same one is added (but before the actual removal from the listener list takes place), then ignored again. In this case, because the delete listener callback is called immediately upon ignore (and not when the actual removal takes place), you will observe the listener being deleted twice, although it should have been deleted just once (because of the CODAL optmization that recovers the ignored listener before it is actually removed).

Scenario:

  • listen(e,v,handler)
  • some time later, ignore(e,v,handler), calls delete handler eagerly
  • then, very quickly listen(e,v,handler), restores the triple
  • some time later, ignore(e,v,handler). calls delete handler a second time,

@abchatra abchatra requested a review from Copilot November 21, 2025 17:17
Copilot finished reviewing on behalf of abchatra November 21, 2025 17:19
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a concurrency bug in the MessageBus listener deletion mechanism. The issue occurs when a listener is ignored, then re-added (triggering the CODAL optimization that "resurrects" the listener by clearing its DELETING flag), and then ignored again. Previously, the deletion callback was invoked in the remove() method when marking listeners for deletion, which would cause it to be called twice in this scenario - once for each ignore operation. By moving the callback invocation to deleteMarkedListeners() (where actual deletion occurs), the callback is now correctly invoked only once per actual deletion.

Key Changes

  • Moved listener_deletion_callback invocation from the remove() method to deleteMarkedListeners() method
  • Callback now executes just before the listener is actually deleted, rather than when it's marked for deletion
  • This ensures the callback is invoked exactly once per actual deletion, even when listeners are resurrected via the CODAL optimization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@thomasjball
Copy link
Author

AI summary is very good and gives more precise details.

@thomasjball
Copy link
Author

thomasjball commented Nov 22, 2025

@carlosperate and @finneyj - can we get a new build of codal-microbit-v2 incorporating this change so we can move pxt-microbit beta to it? I have test it with pxt-microbit, where we enable the delete listeners callback: microsoft/pxt-microbit#6658. It does indeed work nicely.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant