strophe / strophejs

Strophe.js is an XMPP library for JavaScript
http://strophe.im/strophejs
MIT License
1.45k stars 358 forks source link

Wrong order of received bosh responses #709

Open damencho opened 1 month ago

damencho commented 1 month ago

Hey Jitsi Team here. We ran into issue where we were receiving stanzas in wrong order for bosh.

Here is a description of our observation:

  1. We have an open long-poll request with rid=1
  2. We send a presence to a MUC in a new request with rid=2. We expect to receive back a presence, followed shortly by an IQ.
  3. The remote side sends a presence in rid=1, and an IQ in rid=2
  4. Strophe fires the IQ handler first (received in rid=2), and then the presence handler (received in rid=1)

We expect that in 4 strophe should fire the presence (rid=1) first and then the IQ (rid=2). We suspect the issue happens when the rid=2 request completes before the rid=1 request.

And according to XEP-0124 the client needs to order those before firing the listeners The client MUST process responses received from the connection manager in the order the requests were made..

jcbrand commented 1 month ago

There is a _requests array attribute on the Bosh instance and requests are pushed to this array in various places.

To make sure that their responses are processed in the order in which they were inserted, one can create a chain of promises.

So a request that is inserted later, needs to wait for the promises of all the previous requests to be finished before its own handler is called.

To start, this._requests needs to be a promise and not an array.

this._requests = Promise.resolve();

Then, instead of calling this_.requests..push to add new requests (like here), .then() is called to add a new promise.

this._requests.then(() => {
    return new Promise((resolve, reject) => {
        new Request(
             body.tree(),
             () => this._onRequestStateChange(resolve, reject, this._conn._dataRecv.bind(this._conn)),
             Number(body.tree().getAttribute('rid'))
        )
   });
);

_onRequestStateChange would have to be changed to take the resolve and reject functions and to call them at the appropriate times.

saghul commented 1 month ago

Do you reckon it's possible that due to the fact that BOSH uses more than 1 connection requests could come out of order under certain conditions?

If that is indeed possible, would it make sense to keep an array, do ordered inserts by rid, and have an async function be picking requests off the array one by one?

jcbrand commented 1 month ago

Do you reckon it's possible that due to the fact that BOSH uses more than 1 connection requests could come out of order under certain conditions?

Requests should be added to the promise chain in the order in which they are sent out, not in the order in which their responses are received. So it shouldn't matter if responses are received in the wrong order.

However, as I write this, I realize my proposal would serialize requests and make it impossible to have parallel requests (because subsequent requests are waiting for previous ones to finish). This is a dealbreaker.

What's needed is to serialize responses, or at least to serialize the calling of the handlers (func(req)). And this serialization needs to be done when the requests are created, in order to maintain the right order.

That way requests are sent out in parallel, but responses are handled in the right order and each response waits for the previous one to finish.

I believe this is doable, but I don't have the time to pursue this further right now.