Skip to content

Conversation

@ruffin--
Copy link

Made/Proposed changes:

QB.disconnect followed by QB.connect can cause event handlers to be registered with Strophe multiple times. For instance, if a user has assigned a function myMessageListener via QB.chat.onMessageListener = myMessageListener; that function would be called twice for the same new message after a QB.connect, then QB.disconnect and second QB.connect.

This pull request fixes this issue by tracking references to event handlers via an extension method placed on the Strophe connection. It then removes those existing event handlers from Strophe during a second call to ChatProxy.connect that enters the case Strophe.Status.CONNECTED: block.

Checkin info below:

Strophe returns references to handlers when they are added via .addHandler that must be maintained to remove those via deleteHandler.
http://strophe.im/strophejs/doc/1.2.10/files/strophe-js.html#Strophe.Connection.deleteHandler

Disconnecting via QuickBlox and later reconnecting would add new handlers without removing existing handlers, which would cause events to have multiple copies of their listeners.

Adding extension methods XAddTrackedHandler & XDeleteHandlers onto the Strophe Connection immediately after creation in qbMain, replacing all addHandler calls with XAddTrackedHandler, and then deleting those event handlers in qbChat's connect function's CONNECTED switch block with XDeleteHandlers is a low-churn solution that eliminates this problem.


How should this be manually tested?

####Fail case

  1. Comment out the line connection.XDeleteHandlers(); in qbChat's connect method (line 563 in pull request) so that it is not executed.
  2. Register an event handler for the message listener.
    • QB.chat.onMessageListener = function(msg) { console.log(JSON.stringify(msg)); };
  3. Init and then create a QuickBlox session.
  4. Execute a successful QB.disconnect command followed by a QB.reconnect command with the same user used in step 3.
  5. Send logged in QuickBlox user a message from another logged in user.
  6. The message listener function will fire twice. This is unexpected behavior.

####Success Case

  1. Remove comment marker from the line connection.XDeleteHandlers(); in qbChat's connect method (line 563 in pull request) so that it does fire.
  2. Continue to register an event handler for the message listener.
    • QB.chat.onMessageListener = function(msg) { console.log(JSON.stringify(msg)); };
  3. Init and then create a QuickBlox session.
  4. Execute a successful QB.disconnect command followed by a QB.reconnect command with the same user used in step 3.
  5. Send logged in QuickBlox user a message from another logged in user.
  6. The message listener function will only fire once, as expected.

Does the documentation need an update?

No.

ruffin-- added 3 commits May 11, 2017 10:54
Strophe returns references to handlers when they are added via .addHandler that must be maintained to remove those via deleteHandler.
http://strophe.im/strophejs/doc/1.2.10/files/strophe-js.html#Strophe.Connection.deleteHandler

Disconnecting via QuickBlox and later reconnecting would add new handlers without removing existing handlers, which would cause events to have multiple copies of their listeners.

Adding extension methods XAddTrackedHandler & XDeleteHandlers onto the Strophe Connection immediately after creation in qbMain, replacing all addHandler calls with XAddTrackedHandler, and then deleting those handlers in qbChat's `connect` function's CONNECTED switch block with XDeleteHandlers is a low-churn solution that eliminates this problem.
@soulfly
Copy link
Contributor

soulfly commented May 15, 2017

@ruffin-- thanks man!, we will review this and came back to you

@dimaspirit
Copy link
Contributor

All is fine, it's a good idea. We will release it as 2.6.1 version.
Release in approximately 2 weeks.

@dimaspirit
Copy link
Contributor

@ruffin-- , hey. We updated sdk and I want to merge into develop branch. Could you resolve conflicts? Sorry for this.

@ruffin--
Copy link
Author

ruffin-- commented May 27, 2017

Hey, @dimaspirit , sure can! Took me a second to catch up, but the changes to the sdk make this a cleaner fix too. No more qbMain editing. You could even pull out the extension methods and hide the handler overhead in qbChat scope, but I think the extension methods make for more readable code.

Apologies for taking a while to find some time to merge.

@dimaspirit dimaspirit merged commit d0fbdd7 into QuickBlox:develop May 29, 2017
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.

3 participants