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

Make server respond to websocket ping messages #560

Closed
maximbaz opened this issue Dec 31, 2018 · 11 comments
Closed

Make server respond to websocket ping messages #560

maximbaz opened this issue Dec 31, 2018 · 11 comments

Comments

@maximbaz
Copy link

Webapp sends 'Wire is so much nicer with internet!' every now and then to make sure it is still able to talk to the server. However because server doesn't respond to this message, in some cases it is impossible to detect if webapp is actually offline.

See this message for details: wireapp/wire-desktop#1129 (comment)

Please make Wire respond with some string like 'Indeed it is' on each such ping request, so that it's possible to improve webapp.

@tiago-loureiro
Copy link
Contributor

Hi @maximbaz

Websockets have a built-in mechanism for this using control frames which is already supported on the backend (check this)

@bennyn Perhaps you want to have a look at this?

@maximbaz
Copy link
Author

maximbaz commented Jan 4, 2019

Hey hey @tiago-loureiro 👋

Yes I know about those, unfortunately they are not supported by client-side javascript, that's the reason webapp is faking ping requests by sending strings to the server.

https://www.w3.org/TR/websockets/#ping-and-pong-frames

The WebSocket protocol specification defines Ping and Pong frames that can be used for keep-alive, heart-beats, network status probing, latency instrumentation, and so forth. These are not currently exposed in the API.

@fisx fisx mentioned this issue Jan 7, 2019
2 tasks
@fisx
Copy link
Contributor

fisx commented Jan 7, 2019

Working on it. Thanks @maximbaz for reporting this, and for not letting go! :-)

@fisx
Copy link
Contributor

fisx commented Jan 8, 2019

I just talked with @bennyn:

  • yes, the browser gives js no chance of learning about the death of its websocket other than the payload-level ping/pong game.
  • we don't want to use protobuf to implement this like with all the other websocket traffic. the backend dislikes the beaurocracy involved, and the web client would have to pay a performance penalty.
  • instead, the message payloads are ping and pong (those 4 bytes).
  • all messages other than ping sent by the client will be ignored as before. this guarantees backwards compatibility (currently, the web client never sends ping).
  • this plan needs to be confirmed by the other clients.

@maximbaz
Copy link
Author

maximbaz commented Jan 8, 2019

Solid plan 👍 Thank you both!

@marcoconti83
Copy link
Member

To my understanding, this is an optional addition to the protocol. Only if a client sends a ping, the backend will respond with a pong. Currently no clients sends a ping so this addition is safe for older clients.

fisx added a commit that referenced this issue Jan 16, 2019
This reverts commit eeaeb4c.

The problem with #560 was that the permission I picked to
reuse was one that members did not have, but from here on
we expected them to.
fisx added a commit that referenced this issue Jan 16, 2019
* Revert "Change of team permission semantics (#569)"

This reverts commit eeaeb4c.

The problem with #560 was that the permission I picked to
reuse was one that members did not have, but from here on
we expected them to.

* Change of team permission semantics (Fixes #569).
@fisx fisx closed this as completed in #561 Jan 20, 2019
@maximbaz
Copy link
Author

Yay for getting this merged! 🙂 @bennyn ping me if you want an early testing, or even for implementation if you guys are busy with other stuff, this is my most annoying issue with Wire on Desktop so I would be more than happy to collaborate again and get this implemented quickly 😉

@bennycode
Copy link

@maximbaz PRs are more than welcome because we won't have time this month to implement it ourselves.

@fisx
Copy link
Contributor

fisx commented Jan 21, 2019

@maximbaz also note that the PR went to staging yesterday; the next release that will bring it to app.wire.com is still a few days off (not scheduled, in fact, but very likely due this month).

btw i really appreciate your patience and helpful attitude. it's great working with you! :-)

@maximbaz
Copy link
Author

Thanks for the kind words, happy to collaborate with you too 🙂 I have some preliminary implementation, please ping me when the code reaches app.wire.com so I can do some final testing before submitting a PR.

@fisx
Copy link
Contributor

fisx commented Jan 28, 2019

@maximbaz now this ticket is legitimately closed, and #561 is in production.

please re-open (or ping me) if you need anything else from the backend.

jschaul added a commit that referenced this issue Feb 27, 2023
Introduces an integration test / regression test to check that control-level pings with a payload result in a control-level pong with the same payload as specified in the [RFC](https://www.rfc-editor.org/rfc/rfc6455#section-5.5.2)

This was used in debugging https://wearezeta.atlassian.net/browse/FS-1489

(related ping-pong prior work: #561 and prior discussion: #560)
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

No branches or pull requests

5 participants