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.

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

Fix websocket reconnect logic #242

Merged
merged 3 commits into from
Nov 28, 2019
Merged

Fix websocket reconnect logic #242

merged 3 commits into from
Nov 28, 2019

Conversation

quthla
Copy link
Contributor

@quthla quthla commented Nov 26, 2019

The code is obsolete now that there is the improved auto reconnect function

It also did never actually work anyway. This will call listen with an undefined callback, thus leading to the following error and effectively preventing the client from reconnecting automatically.

TypeError: e is not a function WebSocketStore.ts:33:33

@quthla quthla requested a review from a team as a code owner November 26, 2019 23:39
@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

Merging #242 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #242   +/-   ##
=======================================
  Coverage   90.87%   90.87%           
=======================================
  Files          43       43           
  Lines        1677     1677           
=======================================
  Hits         1524     1524           
  Misses         82       82           
  Partials       71       71

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9715eca...0df1f52. Read the comment docs.

Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

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

Hey @quthla, thanks for your contribution!

It's a good catch but i don't think this code is obsolete. There are some miss configurations in which the web socket could be closed without having errors on any HTTP request. An example would be a wrongly configured reverse proxy which has a really small read/write/connection timeout. (see proxy_*_timeout settings in https://gotify.net/docs/nginx) In this case, the web socket would be closed without any reconnect.

Could we fix the existing implementation that we still do the reconnect and show the error message?

@jmattheis jmattheis added this to the version+1 milestone Nov 27, 2019
@quthla
Copy link
Contributor Author

quthla commented Nov 27, 2019

It will still call tryAuthenticate on close which fires the reconnect logic so that's not an issue and the code is not needed anymore

@jmattheis
Copy link
Member

Are you sure? I think the reconnect logic (CurrentUser.connectionError) will only be executed if there was an error the first place (tryAuthenticate() will resolve successfully). In this case, only the web socket closed the rest of the state doesn't know of this and still thinks everything is fine.

@quthla
Copy link
Contributor Author

quthla commented Nov 27, 2019

Are you sure? I think the reconnect logic (CurrentUser.connectionError) will only be executed if there was an error the first place (tryAuthenticate() will resolve successfully). In this case, only the web socket closed the rest of the state doesn't know of this and still thinks everything is fine.

We can of course also keep the code.

@jmattheis
Copy link
Member

It's not an obvious error, normal users doesn't look in the console for errors, so we don't know how frequent the bug occurs.

@quthla
Copy link
Contributor Author

quthla commented Nov 27, 2019

Well, if you want to keep it, then that's ok with me. After all, you know the backend and how all is tied together better than me.

I've restored and fixed the old code.

@quthla quthla changed the title Remove obsolete and broken websocket reconnect Fix websocket reconnect logic Nov 27, 2019
Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

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

Perfect, thank you!

@jmattheis jmattheis merged commit dc74149 into gotify:master Nov 28, 2019
@quthla
Copy link
Contributor Author

quthla commented Nov 28, 2019

Can you make a new release? Maybe gotify will then finally reliably reconnect for me 😂

@jmattheis
Copy link
Member

jmattheis commented Nov 28, 2019

Do you use a reverse proxy? Gotify only closes the websocket connection if the user/client gets deleted, every other close should not happen.

@quthla
Copy link
Contributor Author

quthla commented Nov 28, 2019

I think it's more of a network issue which rarely occurs but yes I'm using a reverse proxy.

Checked my proxy's error logs but nothing there

@jmattheis
Copy link
Member

Could you check the gotify log if there is something regarding websockets? Otherwise, it's most likely the fault of the reverse proxy. But, I'll create a new release after fixing #243

@quthla
Copy link
Contributor Author

quthla commented Nov 28, 2019

No results grepping for websocket. Grepping error yields a few hundred lines like the following

gotify | Error #01: you need to provide a valid access token or user credentials to access this api

@jmattheis
Copy link
Member

That seems normal, I'll add some better error messages for websockets. With d764a8d messages like WebSocket: ReadError websocket: close 1006 (abnormal closure): unexpected EOF will be logged. This hopefully makes it clear, on which end the error occurs.

@jmattheis
Copy link
Member

@quthla
Copy link
Contributor Author

quthla commented Dec 1, 2019

@jmattheis

gotify    | WebSocket: ReadError read tcp 172.17.0.3:80->172.17.0.1:52362: i/o timeout
gotify    | WebSocket: ReadError read tcp 172.17.0.3:80->172.17.0.1:52378: i/o timeout
gotify    | Error #01: you need to provide a valid access token or user credentials to access this api
gotify    | Error #01: you need to provide a valid access token or user credentials to access this api
gotify    | WebSocket: ReadError websocket: close 1006 (abnormal closure): unexpected EOF
gotify    | WebSocket: ReadError websocket: close 1006 (abnormal closure): unexpected EOF
gotify    | Error #01: you need to provide a valid access token or user credentials to access this api
gotify    | WebSocket: ReadError websocket: close 1006 (abnormal closure): unexpected EOF
gotify    | Error #01: you need to provide a valid access token or user credentials to access this api
gotify    | WebSocket: ReadError websocket: close 1006 (abnormal closure): unexpected EOF
gotify    | Error #01: you need to provide a valid access token or user credentials to access this api

@quthla quthla deleted the patch-1 branch December 1, 2019 19:29
@jmattheis
Copy link
Member

@quthla Could you open a new issue for this and don't forget to include your nginx config.

@quthla
Copy link
Contributor Author

quthla commented Dec 1, 2019

I'm using Apache. So you suspect this to be an issue with gotify or why should I open a new issue?

@jmattheis
Copy link
Member

Most likely not, but without the config I can't say it for sure. I don't want to discuss this issue in a pull request, therefore you've to create a new issue for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants