-
Notifications
You must be signed in to change notification settings - Fork 404
fix(internal-plugin-mercury): improve handling of 4000 close code #5065
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
base: next
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -901,9 +901,23 @@ const Mercury = WebexPlugin.extend({ | |
| break; | ||
| case 4000: | ||
| // metric: disconnect | ||
| this.logger.info(`${this.namespace}: socket ${sessionId} replaced; will not reconnect`); | ||
| if (isActiveSocket) this._emit(sessionId, 'offline.replaced', event); | ||
| // If not active, nothing to do | ||
| if (isActiveSocket) { | ||
| // 4000 received directly from Mercury on the currently active socket. | ||
| // Treat as a transient disconnect and reconnect to recover. | ||
| this.logger.info( | ||
| `${this.namespace}: active socket ${sessionId} closed with 4000; reconnecting` | ||
| ); | ||
| this._emit(sessionId, 'offline.transient', event); | ||
|
mkesavan13 marked this conversation as resolved.
|
||
| this._reconnect(socketUrl, sessionId); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a 4000 close is caused by another tab/process replacing this Mercury connection, this socket is still the active socket inside the current SDK instance because the replacement is not in Useful? React with 👍 / 👎. |
||
| } else { | ||
| // 4000 received on an old/replaced connection (e.g. the SDK connected | ||
| // twice). The old socket is closed; the active connection remains and | ||
| // no reconnect should be triggered. | ||
| this.logger.info( | ||
| `${this.namespace}: old socket ${sessionId} replaced; will not reconnect` | ||
| ); | ||
| this._emit(sessionId, 'offline.replaced', event); | ||
|
Comment on lines
+916
to
+919
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an old socket closes with 4000 after a newer socket has been stored for the same session, Useful? React with 👍 / 👎. |
||
| } | ||
| break; | ||
| case 4001: | ||
| // replaced during shutdown | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't look right - we're changing the behavior for the
isActiveSocketcase. isActiveSocket is primarily for handling LLM where we can have multiple LLM connections, for Mercury we always have just one, so you would never have isActiveSocket==false for Mercury.Also the testing done in the vidcast is not representative of the real world scenario, because it's setting up both mercury connections with same WDM in the same SDK instance. In real life it's 2 web apps running in separate windows, so 2 separate SDK instances each one has just a single Mercury connection.