stream-management: Implement requesting ACKs #1005

singpolyma · 2023-11-14T22:06:24Z

Queue outgoing stanzas and periodically request ACK. Remove from the queue anything ack'd and notify of the ack so apps can know the stanza has for sure sent.

On resume, anything not ack'd is re-sent. On reconnect, anything not ack'd notify of the failure to send this stanza so apps can know the stanza failed.

Even when there is no traffic, send an at least every 5 minutes to check the connection. If there is no inbound traffic (such as an ) within timeout (default 60 seconds) then consider the connection disconnected.

sonnyp

Thank you

Tests are needed, in particular because this implements quite a complex behavior

In general I try to avoid "internal" XMPP events on entity . They also don't appear to be used. Are they needed? What for and in which case they should be documented. Possibly also emitted on the object returned by function streamManagement

packages/xml/index.js

packages/stream-management/index.js

package.json

singpolyma · 2024-12-18T21:07:41Z

In general I try to avoid "internal" XMPP events on entity . They also don't appear to be used. Are they needed?

Yes I am using these events in the snikket SDK to eg know when a message has been acknowledged or not, so I can update my state to know if it made it to the server. Most apps even show this status information (eg one check in conversations means "stream management ack")

Queue outgoing stanzas and periodically request ACK. Remove from the queue anything ack'd and notify of the ack so apps can know the stanza has for sure sent. On resume, anything not ack'd is re-sent. On reconnect, anything not ack'd notify of the failure to send this stanza so apps can know the stanza failed. Even when there is no traffic, send an <r/> at least every 5 minutes to check the connection. If there is no inbound traffic (such as an <a/>) within timeout (default 60 seconds) then consider the connection disconnected.

…eview)

packages/stream-management/index.js

sonnyp · 2025-01-08T09:58:39Z

packages/stream-management/index.js

+      !qStanza.getChild("delay", "urn:xmpp:delay")
+    ) {
+      qStanza = xml.clone(qStanza);
+      qStanza.c("delay", {


please use append

packages/stream-management/index.js

sonnyp · 2025-01-08T10:01:04Z

packages/stream-management/index.js

+    let q = sm.outbound_q;
+    sm.outbound_q = [];
+    for (const item of q) {
+      await entity.send(item); // This will trigger the middleware and re-add to the queue


what if entity.send rejects ?

Then resumed will fail. What context causes send to reject?

sonnyp · 2025-01-08T10:02:04Z

packages/stream-management/stream-features.test.js

+  expect(entity.streamManagement.outbound_q).toHaveLength(1);
+  expect(
+    entity.streamManagement.outbound_q[0].getChild("delay", "urn:xmpp:delay"),
+  ).not.toBeUndefined();


please move this to a new test

packages/stream-management/stream-features.test.js

sonnyp · 2025-01-08T10:07:27Z

Also

Update stream management README
Add references to the spec as comments where relevant
Add an e2e test ( see /test/sasl.js for example)

singpolyma · 2025-01-08T15:59:41Z

Add references to the spec as comments where relevant

looks like link to xep is already there. was there something else you're looking for?

sonnyp

looks like link to xep is already there. was there something else you're looking for?

direct quotes

sonnyp · 2025-01-09T11:14:56Z

packages/stream-management/index.js

+    // Debounce requests so we send only one after a big run of stanza together
+    clearTimeout(requestAckTimeout);
+    requestAckTimeout = setTimeout(requestAck, sm.debounceAckRequest);


xmpp.js has a sendMany method, could we hook to that instead (if not in this PR then later)?

packages/stream-management/index.js

test/stream-management.js

packages/stream-management/stream-features.test.js

sonnyp · 2025-01-09T11:30:45Z

packages/stream-management/index.js

+    let item;
+    while ((item = sm.outbound_q.shift())) {
+      sm.emit("fail", item.stanza);
+    }


wdyt of

Suggested change

let item;

while ((item = sm.outbound_q.shift())) {

sm.emit("fail", item.stanza);

}

sm.emit("fail", sm.outbound_q);

sm.outbound_q = [];

That feels like it would make the library internals very slightly simpler in exchange for making the API slightly more complex? Is there a reason you're interested in this change?

As it stands xmpp.js is a fairly low level XMPP library. I prefer abstractions to reflect the protocols to leave room for optimizations.

If that changes then we can think of what the API should be like and deal with persistence, tab reload etc

sonnyp · 2025-01-09T11:33:33Z

packages/stream-management/index.js

+    for (let i = 0; i < resumed.attrs.h - oldOutbound; i++) {
+      let item = sm.outbound_q.shift();
+      sm.outbound++;
+      sm.emit("ack", item.stanza);
+    }


wdyt of something like that instead

Suggested change

for (let i = 0; i < resumed.attrs.h - oldOutbound; i++) {

let item = sm.outbound_q.shift();

sm.outbound++;

sm.emit("ack", item.stanza);

}

const items = sm.outbound_q.slice(-resumed.attrs.h);

sm.outbound += resumed.attrs.h;

sm.emit("ack", items);

packages/stream-management/index.js

Store the stamp seperately in the queue and then add it the first time we retry. Saves some work and a dependency.

Needs to use force-disconnect especially if we emulate an issue using pause

sonnyp · 2025-01-15T12:16:35Z

#1054

sonnyp reviewed Dec 12, 2024

View reviewed changes

packages/xml/index.js Outdated Show resolved Hide resolved

packages/stream-management/index.js Outdated Show resolved Hide resolved

sonnyp reviewed Dec 12, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

sonnyp mentioned this pull request Jan 7, 2025

Cannot read properties of null (reading 'once') in 'promise.js' #974

Open

singpolyma force-pushed the implement-sm-for-outgoing branch from 01f2a17 to c4d4bb8 Compare January 7, 2025 17:01

sonnyp added 3 commits January 8, 2025 10:13

Emit events on streamManagement object as suggested on xmppjs#1005 (r…

6fa3127

…eview)

Remove unecessary conditions to clearTimeout

710b167

prefer early return

ce30e8b

sonnyp reviewed Jan 8, 2025

View reviewed changes

sonnyp mentioned this pull request Jan 8, 2025

Does not reconnect when disconnect isn't clean #993

Open

sonnyp changed the title ~~Implement stream management requesting ACKs~~ stream-management: Implement requesting ACKs Jan 9, 2025

sonnyp reviewed Jan 9, 2025

View reviewed changes

singpolyma added 15 commits January 13, 2025 11:17

No more clone

7ef1d8d

Store the stamp seperately in the queue and then add it the first time we retry. Saves some work and a dependency.

Throw Error not string

d76e479

Catch any disconnect failure

69d0752

Reformat this line

0190cb6

Make more timeouts configurable

b2bf99c

Test failure with something in queue seperately

0f79b3b

Document events in readme

efad5c1

Fix comment position

8cb8dff

e2e test for stream management

8c408ef

Use sendMany

43a2d69

Change test name

97e8d28

Split up resumed test

f076fd1

Teardown on disconnect

f6e15ba

Use promise helper

2209036

e2e test for fail event when offline

c66696c

singpolyma added 2 commits January 13, 2025 11:51

e2e test for stanza retry

bb3998a

e2e test auto reconnect

4736120

Needs to use force-disconnect especially if we emulate an issue using pause

singpolyma force-pushed the implement-sm-for-outgoing branch from 49e0fe1 to 4736120 Compare January 13, 2025 17:31

sonnyp mentioned this pull request Jan 15, 2025

stream-management: Implement requesting ACKs #1054

Merged

sonnyp closed this Jan 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream-management: Implement requesting ACKs #1005

stream-management: Implement requesting ACKs #1005

singpolyma commented Nov 14, 2023

sonnyp left a comment

singpolyma commented Dec 18, 2024

sonnyp Jan 8, 2025

sonnyp Jan 8, 2025

singpolyma Jan 8, 2025

sonnyp Jan 8, 2025

sonnyp commented Jan 8, 2025 •

edited by singpolyma

Loading

singpolyma commented Jan 8, 2025 •

edited

Loading

sonnyp left a comment

sonnyp Jan 9, 2025

sonnyp Jan 9, 2025

singpolyma Jan 13, 2025

sonnyp Jan 15, 2025

sonnyp Jan 9, 2025

sonnyp commented Jan 15, 2025

stream-management: Implement requesting ACKs #1005

stream-management: Implement requesting ACKs #1005

Conversation

singpolyma commented Nov 14, 2023

sonnyp left a comment

Choose a reason for hiding this comment

singpolyma commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonnyp commented Jan 8, 2025 • edited by singpolyma Loading

singpolyma commented Jan 8, 2025 • edited Loading

sonnyp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonnyp commented Jan 15, 2025

sonnyp commented Jan 8, 2025 •

edited by singpolyma

Loading

singpolyma commented Jan 8, 2025 •

edited

Loading