CRNS-264: Connection related fixes/changes #626

vishalnarkhede · 2021-03-06T19:40:39Z

Submit a pull request

Call setHealth(true) only after you receive first ws event (so moved from onopen to onmessage). Only after onmessage, we can be sure that connection is ready. This is mentioned in one of the existing code comments as well:

we wait till the first message before we consider the connection open..
the reason for this is that auth errors and similar errors trigger a ws.onopen and immediately
after that a ws.onclose.

Introducing this change, because we don't get connectionId from onopen callback on websocket. We only get
connectionId from onmessage callback. So with existing implementation, if you want to rewatch/requery channels upon
network recovery, you can't rely on health of websocket to do that. Because watching channels requires connectionId.

After the change, you can easily do following:

client.on('connection.changed', (event) => {
  if (event.online) {
    // rewatch your channels or implement your recovery logic
  }
})

client.reconnectUser();

Introduced two aliases
- client.closeConnection for client.wsConnection.disconnect // Updated description
- client.openConnection for client._setupConnection // Updated description
This makes it easier for push notification handling - as mentioned in caveats
Removed client.connectionId to avoid hassle of keeping it in sync with client.wsConnection.connectionId. Lets only access it from client.wsConnection.connectionId moving forward

TODO

Run unit tests
Run integration tests

CLA

I have signed the Stream CLA (required).
The code changes follow best practices
Code changes are tested (add some information if not applicable)

Description of the pull request

github-actions · 2021-03-06T19:42:41Z

Size Change: +2.35 kB (1%)

Total Size: 216 kB

Filename	Size	Change
`dist/browser.es.js`	47 kB	+563 B (1%)
`dist/browser.full-bundle.min.js`	27.3 kB	+143 B (0%)
`dist/browser.js`	47.5 kB	+539 B (1%)
`dist/index.es.js`	47 kB	+564 B (1%)
`dist/index.js`	47.5 kB	+543 B (1%)

_{compressed-size-action}

thesyncim

LGTM

thesyncim · 2021-03-07T20:01:28Z

src/connection.ts

@@ -492,6 +491,8 @@ export class StableWSConnection<
        return;
      } else {
        this.resolvePromise?.(event);
+        // set healthy..
+        this._setHealth(true);


this should fix the bug "watch or presence requires an active WebSocket connection"

AnatolyRugalev

LGTM

ferhatelmas

LGTM

…hal/sethealth-fix

….disconnect

…at-js into vishal/sethealth-fix

vishalnarkhede · 2021-03-08T14:15:58Z

@AnatolyRugalev @ferhatelmas @thesyncim

Updated some aliases after discussion with @mahboubii :

connectUser - connects a user
disconnectUser - disconnects a user, without removing user or its data from client
reconnectUser - reconnects a current user
disconnect - completely destroys entire session/client. We should probably rename this to client.destroy or something.

mahboubii · 2021-03-08T15:52:01Z

src/client.ts

-    if (this.wsConnection) {
-      return this.wsConnection.disconnect(timeout);
-    }
+    this.disconnectUser(timeout);


this needs to be run on the function entry point before we reset the activeChannels state

Suggested change

this.disconnectUser(timeout);

const returnPromise = this.disconnectUser(timeout);

.....

cleanup client

...

return returnPromise;

vishalnarkhede · 2021-03-09T18:51:42Z

Ok so finalising following endpoints as per discussion:

connectUser
disconnectUser (alias for disconnect)
closeConnection
openConnection

Will add these changes tomorrow !!

…hal/sethealth-fix

client._setupConnection -> client.openConnection client.wsConnection.disconnect -> client.closeConnection client.connectUser -> client.connectUser client.disconnect -> client.disconnectUser

…at-js into vishal/sethealth-fix

mahboubii · 2021-03-10T12:22:36Z

src/client.ts

+  };
+
+  /**
+   * Reconnects the current user.


Suggested change

* Reconnects the current user.

* Creates a new WebSocket connection with the current user. throws if there is an active connection

src/client.ts

mahboubii

LGTM 🌮

- #635 - #626

vishalnarkhede added 2 commits March 6, 2021 20:27

Fixing the setHealth call position in ws connection logic

771104f

Introduced aliases for disconnect and reconnecting websocket on client

dca08b3

vishalnarkhede requested review from nhannah, mahboubii, tbarbugli, ferhatelmas and thesyncim March 6, 2021 19:40

vishalnarkhede changed the title ~~Connection related fixes/changes~~ CRNS-264: Connection related fixes/changes Mar 6, 2021

vishalnarkhede requested a review from AnatolyRugalev March 6, 2021 19:51

Added docs

8792dbf

vishalnarkhede requested a review from DanC5 March 6, 2021 19:56

vishalnarkhede added 2 commits March 6, 2021 20:57

Updated alias to return the internal call

12927a1

Set isUpToDate flag to false, when websocket is disconnected

f21a946

vishalnarkhede mentioned this pull request Mar 6, 2021

CRNS-264: Fixing issues around network recovery GetStream/stream-chat-react-native#544

Merged

3 tasks

thesyncim previously approved these changes Mar 7, 2021

View reviewed changes

thesyncim reviewed Mar 7, 2021

View reviewed changes

AnatolyRugalev reviewed Mar 8, 2021

View reviewed changes

AnatolyRugalev previously approved these changes Mar 8, 2021

View reviewed changes

ferhatelmas previously approved these changes Mar 8, 2021

View reviewed changes

ferhatelmas and others added 4 commits March 8, 2021 13:28

Merge branch 'master' into vishal/sethealth-fix

a05d8dc

Merge branch 'master' of github.com:GetStream/stream-chat-js into vis…

2b15f97

…hal/sethealth-fix

Added new endpoint - client.disconnectUser, which is alias for client…

7735cd5

….disconnect

Merge branch 'vishal/sethealth-fix' of github.com:GetStream/stream-ch…

17c9c02

…at-js into vishal/sethealth-fix

vishalnarkhede dismissed stale reviews from ferhatelmas, AnatolyRugalev, and thesyncim via 17c9c02 March 8, 2021 13:35

Updating reconnect/disconnect endpoints

33de312

Merge branch 'master' into vishal/sethealth-fix

3527ac1

AnatolyRugalev previously approved these changes Mar 8, 2021

View reviewed changes

mahboubii reviewed Mar 8, 2021

View reviewed changes

Merge branch 'master' into vishal/sethealth-fix

35827ef

vishalnarkhede added 6 commits March 9, 2021 19:51

Merge branch 'master' into vishal/sethealth-fix

c2af838

Merge branch 'master' into vishal/sethealth-fix

29b50db

Merge branch 'master' of github.com:GetStream/stream-chat-js into vis…

955a79a

…hal/sethealth-fix

Making connection-disconnection methods zero-sum

0fe8b2e

client._setupConnection -> client.openConnection client.wsConnection.disconnect -> client.closeConnection client.connectUser -> client.connectUser client.disconnect -> client.disconnectUser

Merge branch 'vishal/sethealth-fix' of github.com:GetStream/stream-ch…

6a4f2d3

…at-js into vishal/sethealth-fix

Cleanup

6cac247

vishalnarkhede dismissed AnatolyRugalev’s stale review via 6cac247 March 10, 2021 09:33

mahboubii reviewed Mar 10, 2021

View reviewed changes

vishalnarkhede changed the base branch from master to dev March 10, 2021 13:47

vishalnarkhede force-pushed the vishal/sethealth-fix branch from c399521 to 9163e77 Compare March 10, 2021 16:23

Adding extra check for existing connection in openConnection method

5c5e352

vishalnarkhede force-pushed the vishal/sethealth-fix branch from 9163e77 to 5c5e352 Compare March 10, 2021 16:24

mahboubii approved these changes Mar 10, 2021

View reviewed changes

vishalnarkhede merged commit cbe2b42 into dev Mar 10, 2021

vishalnarkhede deleted the vishal/sethealth-fix branch March 10, 2021 16:35

vishalnarkhede mentioned this pull request Mar 10, 2021

Dev #638

Merged

2 tasks

vishalnarkhede added a commit that referenced this pull request Mar 10, 2021

Merge pull request #638 from GetStream/dev

afd1326

- #635 - #626

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRNS-264: Connection related fixes/changes #626

CRNS-264: Connection related fixes/changes #626

vishalnarkhede commented Mar 6, 2021 •

edited

Loading

github-actions bot commented Mar 6, 2021 •

edited

Loading

thesyncim left a comment

thesyncim Mar 7, 2021

AnatolyRugalev left a comment

ferhatelmas left a comment

vishalnarkhede commented Mar 8, 2021 •

edited

Loading

mahboubii Mar 8, 2021 •

edited

Loading

vishalnarkhede commented Mar 9, 2021 •

edited

Loading

mahboubii Mar 10, 2021

mahboubii left a comment

-    this.disconnectUser(timeout);
+    const returnPromise = this.disconnectUser(timeout);
+    .....
+    cleanup client
+    ...
+    return returnPromise;

	* Reconnects the current user.
	* Creates a new WebSocket connection with the current user. throws if there is an active connection

CRNS-264: Connection related fixes/changes #626

CRNS-264: Connection related fixes/changes #626

Conversation

vishalnarkhede commented Mar 6, 2021 • edited Loading

Submit a pull request

TODO

CLA

Description of the pull request

github-actions bot commented Mar 6, 2021 • edited Loading

thesyncim left a comment

Choose a reason for hiding this comment

thesyncim Mar 7, 2021

Choose a reason for hiding this comment

AnatolyRugalev left a comment

Choose a reason for hiding this comment

ferhatelmas left a comment

Choose a reason for hiding this comment

vishalnarkhede commented Mar 8, 2021 • edited Loading

mahboubii Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

vishalnarkhede commented Mar 9, 2021 • edited Loading

mahboubii Mar 10, 2021

Choose a reason for hiding this comment

mahboubii left a comment

Choose a reason for hiding this comment

vishalnarkhede commented Mar 6, 2021 •

edited

Loading

github-actions bot commented Mar 6, 2021 •

edited

Loading

vishalnarkhede commented Mar 8, 2021 •

edited

Loading

mahboubii Mar 8, 2021 •

edited

Loading

vishalnarkhede commented Mar 9, 2021 •

edited

Loading