Skip to content

feat(connection): Adds token refresh and cancel resume.#2977

Open
damencho wants to merge 3 commits intomasterfrom
authentication-update
Open

feat(connection): Adds token refresh and cancel resume.#2977
damencho wants to merge 3 commits intomasterfrom
authentication-update

Conversation

@damencho
Copy link
Member

@damencho damencho commented Feb 6, 2026

No description provided.

@damencho damencho force-pushed the authentication-update branch from 56c528f to 82645cc Compare February 6, 2026 22:11
@damencho damencho force-pushed the authentication-update branch from 82645cc to cc14194 Compare February 6, 2026 22:19
Copy link
Member

@hristoterezov hristoterezov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Token Refresh & Cancel Resume

Thanks for working on this. The token refresh feature is a useful addition. I have several concerns — a couple of bugs, some design issues, and an architectural suggestion for a cleaner approach.

See inline comments for details.

this._stropheConn.service = serviceUrl;

// this will trigger a resume
this._stropheConn._doDisconnect(TOKEN_REFRESH);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition: listener registered after disconnect fires

_doDisconnect(TOKEN_REFRESH) is called before the Promise listener is set up (line 821). The chain is synchronous:

_doDisconnect → interceptor → original _doDisconnect_changeConnectStatus(DISCONNECTED)_stropheConnectionCb_tryResumingConnection(force=true)resumeConnection()streamManagement.resume()

If the resume completes (or errors) before the listener on line 821 is registered, the CONNECTED event is missed and the Promise hangs until the 3s timeout.

The listener should be set up before triggering the disconnect.

this._stropheConn.service = serviceUrl;

// this will trigger a resume
this._stropheConn._doDisconnect(TOKEN_REFRESH);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture: consider encapsulating this in the stream management plugin

Using _doDisconnect with a magic condition string (TOKEN_REFRESH) that flows through the callback chain is fragile and non-obvious. The stream management plugin already owns the resume state and intercepts disconnect — it should expose this capability explicitly.

For example, add a disconnectForResume() method to the stream management plugin:

disconnectForResume(): void {
    if (!this.getResumeToken()) {
        throw new Error('No resume token available');
    }
    if (!this._resuming && this._c.connected && !this._c.disconnecting) {
        this._resumeState = { /* save handlers */ };
        this._storedJid = this._c.jid;
    }
    this._c._data = [];
    this._originalDoDisconnect!.apply(this._c);
}

Then refreshToken becomes:

streamManagement.disconnectForResume();

This eliminates the magic condition string, the force parameter on _tryResumingConnection, and the modifications to _stropheConnectionCb. All strophe-internal access stays within the plugin where it already lives.

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