Skip to content

Conversation

@scottf
Copy link
Contributor

@scottf scottf commented Jul 17, 2025

Fixes #1232

Context

The dispatcher api allows subscribe

  1. with a subject and no message handler, subscribe("foo");, in which case it will use the message handler provided when the dispatcher was created.
  2. with a subject plus and a message handler, subscribe("foo", nonDefaultHandler);.

Problem

If you unsubscribe("foo"), the dispatcher currently only unsubscribes the subscription with the default handler, meaning if you had subscribed with a non default handler, that subscriptions did not get unsubscribed. (That was the behavior reported in #1232)

Desired Behavior

The dispatcher should unsubscribe for all handlers, default or otherwise, for a subject.

Considerations

You can only have one subscription for a subject with the default handler (otherwise you just get multiple copies of the same message to that handler) but you can have as many subscriptions for a subject if you provide a non default handler.

So if you do this:

subscribe("foo"); 
subscribe("foo"); 
subscribe("foo", handler1); 
subscribe("foo", handler2);

you end up with 3 subscriptions.

Solution

Unsubscribe all subscriptions for the subject regardless of the handler.

@scottf scottf requested a review from MauriceVanVeen July 17, 2025 19:05
Copy link
Member

@MauriceVanVeen MauriceVanVeen left a comment

Choose a reason for hiding this comment

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

LGTM

I suppose this is a breaking change though. Just like the test, you could subscribe on the same subject using different handlers. I don't know why you'd want to do that.. but I guess worth just calling it out in the release notes and not necessarily prompt a major version bump?

@scottf
Copy link
Contributor Author

scottf commented Jul 17, 2025

I considered that this might be a breaking change, but I think it's a bug that no one really noticed before because typically they don't even unsubscribe, or are done with the connection anyway so it's moot.

@scottf
Copy link
Contributor Author

scottf commented Jul 17, 2025

There is also another issue, and that is that it's possible to make a dispatcher without a default handler and then also subscribe without supplying a handler. The message gets dropped. I'm adding throwing an exception in this case, because it's been failing silently forever, now the user will get an exception.

@scottf scottf requested a review from MauriceVanVeen July 17, 2025 19:48
Copy link
Member

@MauriceVanVeen MauriceVanVeen left a comment

Choose a reason for hiding this comment

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

LGTM

@scottf scottf merged commit bc8b991 into main Jul 18, 2025
5 checks passed
@scottf scottf deleted the issue-1232 branch July 18, 2025 12:12
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.

Dispatcher unsubscribe should throw if the wrong method is used

3 participants