Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong order of received bosh responses #709

Open
damencho opened this issue Jun 7, 2024 · 3 comments
Open

Wrong order of received bosh responses #709

damencho opened this issue Jun 7, 2024 · 3 comments

Comments

@damencho
Copy link
Contributor

damencho commented Jun 7, 2024

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
Copy link
Contributor

jcbrand commented Jun 10, 2024

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
Copy link

saghul commented Jun 10, 2024

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
Copy link
Contributor

jcbrand commented Jun 10, 2024

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.

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

No branches or pull requests

3 participants