Strophe.Websocket.prototype._onIdle send redundant packets

[eugeniumegherea]

Found interesting behavior:

test: I was implementing private chat. After a message is sent, I immediately request an acknowledge packet. This happens to be very fast (chrome dev tools says that they are sent exactly at the same time). Because that, _onIdle function gets called 2 times, each with the same packets to be sent. In other words, the _onIdle function does not have enough time to complete and clear queue before next _onIdle call. This results in redundant packets being sent.

expected: No redundant packets being sent

Possible fix: ~~move this._conn._data = []; right under var data = this._conn._data;, so queue gets immediately cleared after a local variable is created.~~

EDIT: (suggested fix was not correct) You need to clear the queue only if status is not paused, so you dont remove unsent stanzas move this._conn._data = []; right under if (data.length > 0 && !this._conn.paused) { line

env: Chrome Version 65.0.3325.181 (Official Build) (64-bit) Using sockets implementation Strophe version: 1.2.14

[jcbrand]

I'm on mobile and can't look at the code right now, but this sounds like a bug. Why can't both stanzas not be sent out in a single _onIdle call?

[eugeniumegherea]

I believe this is how websocket communication is implemented in strophe itself, it sends stanzas right away. As I understood, once you call <connection>.send(xmlElement), it internally will push to queue and immediately will call _onIdle, which will send stanzas and clear queue, no timeouts or waiting whatsoever. I think, without manually push-ing stanzas in the queue, we cannot send 2 stanzas in one _onIdle call.

[jcbrand]

We could debounce _onIdle so that it doesn't get called more than once within 50ms or so. That would ensure that it gets called only once in your case.

[jcbrand]

Thinking about it a bit more, this would probably be better as a leading debounce, so that in most cases when it's called it executes immediately, but if it's called multiple times in short succession, then the calls numbered 2 to n are all executed once after 50ms.

[jcbrand]

I had a chance to look at the code in more detail.

Looking at the docstring for the flush method, it's clear that the original intention was not for the stanza to be sent out immediately when <connection>.send is called.

See here: https://github.com/strophe/strophejs/blob/e81ce285b9be2bacc2bf7c83d1207231427367d9/strophe.js#L3253

So, with that in mind, the solution might be to not flush when calling send. In other words, to make the _send method a no-op:

https://github.com/strophe/strophejs/blob/e81ce285b9be2bacc2bf7c83d1207231427367d9/strophe.js#L6307

[eugeniumegherea]

You might be right, however, I saw a comment showing that flush was indeed intended to be there. I think author thought that there is no need to queue messages in socket connection as there is no benefit from that, so he flushes them right away. (In bosh implementation there is big positive impact queuing things) https://github.com/strophe/strophejs/blob/e81ce285b9be2bacc2bf7c83d1207231427367d9/strophe.js#L6107

Your suggestion might slightly increase latency for socket connection though. What do you think about my initial suggestion? I've done this way in my code and it seems to be working (did not stress test it, just regular use cases)

[jcbrand]

The reason why I was thinking of debouncing etc. is to reduce the cognitive overhead of having the deal with timing issues in the code in _onIdle. For example, code might get refactored or just updated and then the bug appears again.

A good way to avoid that happening though is to add a test for it. So I think you're original proposal is fine, as long as there is a regression test to ensure that we don't mistakenly undo the fix.

Would you mind adding such a test and making a pull request with your fix?

[eugeniumegherea]

I will definitely make a pull request, but not so sure about the test (never wrote a test in my life ¯\_(ツ)_/¯ )