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.

Sign up for GitHub

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

Jump to bottom

Fix can not reconnect websocket after turn off then turn on wifi. Thi… #7552

Merged
merged 3 commits into from
Oct 3, 2023
+8 −0
Merged

Fix can not reconnect websocket after turn off then turn on wifi. Thi… #7552

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
253368d
Fix can not reconnect websocket after turn off then turn on wifi. Thi…
namanh-asher Sep 22, 2023
290a538
Fix can not reconnect websocket after turn off then turn on wifi. Thi…
namanh-asher Sep 26, 2023
108a881
Fix can not reconnect websocket after turn off then turn on wifi. Thi…
namanh-asher Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions app/client/websocket/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ export default class WebSocketClient {
});

this.conn!.onError((evt: any) => {
this.conn = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

why set the connection as undefined on every error? shouldn't this be for a specific error instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@enahum @larkox, there are two reasons why I've added this.conn = undefined here:

  • When I added some logs, I noticed that the initialize function returns early at the line if (this.conn === client).
  • I'm not entirely sure about when onError is called, but if it happens, it means the connection is closed, and we should restart the connection, right? So, I think just reset the local variable

When I examined the logs, I noticed a difference between 4G and Wi-Fi:

  • When I turned off 4G, only onClose is called.
  • When I turned off Wi-Fi, onClose is called, and then, approximately 1 or 2 seconds later, onError is called.
    This difference might depend on the device because I've only encountered this error on Samsung devices. Other Android and iOS devices seem to work fine.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to set the connection to undefined unless needed based on the error, in this case seems that the error is caused by the device not being able to resolve the host. Also you mention that onClose is being called and in that case the connection should be set to undefined on line 185, so it makes even less sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see in the terminal log above, when onClose is called, this.conn is set to undefined, and then initialize is called right after it, updating this.conn. However, 1 or 2 seconds later, onError is called, which triggers another call to initialize. At this point, it returns early because this.conn is the same as before, causing cant reconnect.

I wonder, in the other case of onError, can the WebSocket keep connecting and receive events, or does the WebSocket close every time onError is called?

Copy link
Contributor

Choose a reason for hiding this comment

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

When erroing out depending on the error it could definitely connect again, so I guess you should check yhe type of error

Copy link
Contributor

Choose a reason for hiding this comment

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

So... my concern is what happens if we call conn.open() twice. Will it just be handled on the inners of the library and just ignore if the websocket is already opened or trying to open? Or somehow will "connect twice" and send two onConnect events, etc...? Or will it just directly throw an exception and break the manager?

So... if the answer is the first scenario... we can fire and forget.
For the other two, I would make sure the state of the connection is CLOSED (otherwise, I guess we are connecting somewhere else) and make sure we cancel the retry timeout (so open is not called again due to the retries).

But it would be great if you could verify what is the state of the connection when you get there (probably closed, but since your device is giving weird results, it would be great to verify).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larkox I just tried, use setTimeout for 3 seconds, and then I called conn.open() again when the current readyState is OPEN. It fired onOpen one more time. So maybe we have to check CLOSED.
I cleared this.connectionTimeout, but I'm not sure if there is a timeout to open the WebSocket defined outside of this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any other open should go through the initialize function, and that should handled by the close check you will make, so... I think we are safe on that side.

Thank you very much for checking all these cases for us!

Copy link

Choose a reason for hiding this comment

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

I also faced this issue, just commenting my fix, I just put this condition if the client was same and the client was open, then we would return else, the same flow would continue:

if (this.conn === client && this.conn?.readyState === WebSocketReadyState.OPEN) {
                return
            }

Just putting it out there, if it makes any difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larkox @enahum I just pushed a new commit, plz take a look. I tested it, and it works fine

@MuhammadQasim1122 wow, it look so easy 😄

if (evt.url === this.url) {
this.hasReliablyReconnect = false;
if (this.connectFailCount <= 1) {
Expand Down