Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

Proper Unsubscribe handling, fixes #405 #410

Merged
merged 2 commits into from
Feb 22, 2016

Conversation

behrad
Copy link
Contributor

@behrad behrad commented Feb 6, 2016

  • Before updating ClientSubKey, get current subscriptions and calculate unsubscriptions by comparing the two lists, also publish unscubscriptions so that brokers in cluster can update themselves.
  • Change newSub signature to standard node.js callback style (callback last always)

@@ -252,6 +260,23 @@ RedisPersistence.prototype.storeSubscriptions = function(client, cb) {
});

var op = this._client.multi()
.get(clientSubKey, function(err, currentSubs){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd have preferred to write this with Promises, to merge two publishes, however it won't happen in a single multi, since we need first to get current subscriptions before updating them

@behrad
Copy link
Contributor Author

behrad commented Feb 6, 2016

This is happening for redis persistence, I've not tested mongo & level up ones, to check if this bug also applies to them or not :)

@mcollina
Copy link
Collaborator

mcollina commented Feb 6, 2016

I would suggest that you add a unit test in https://github.com/mcollina/mosca/blob/master/test/persistence/abstract.js. So we can verify if this bug happens everywhere or not.

mcollina added a commit that referenced this pull request Feb 22, 2016
Proper Unsubscribe handling, fixes #405
@mcollina mcollina merged commit 8570cad into moscajs:master Feb 22, 2016
@mcollina
Copy link
Collaborator

Merging and releasing. But I would like to have a proper unit test to cover this.

@behrad
Copy link
Contributor Author

behrad commented Feb 22, 2016

Currently using this in my own fork, tested. But I'll write a containing unit test 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants