App-level websocket pongs. #561
Conversation
services/cannon/src/Cannon/App.hs
Outdated
| _ -> reset counter s 0 >> loop | ||
| _ -> do | ||
| reset counter s 0 | ||
| sendAppLevelPong |
There was a problem hiding this comment.
I feel slightly uneasy about sending a pong for every data message – perhaps we should only pong to messages that say "ping"? Though I can't see any actual problems with sending a pong for every data message.
I also feel uneasy about sending and receiving payloads that are not Protobuf/JSON-encoded (or generally encoded in the same way as other payloads), but dragging Protobuf into this code looks like a mistake.
There was a problem hiding this comment.
I feel slightly uneasy about sending a pong for every data message – perhaps we should only pong to messages that say "ping"? Though I can't see any actual problems with sending a pong for every data message.
the reason i chose to ignore the ping message is that it is currently a rather ad hoc string, and i think it's brittle and dangerous to depend on that. note that we don't expect any other messages but ping; everything else from the client is sent via http. (right?)
I also feel uneasy about sending and receiving payloads that are not Protobuf/JSON-encoded (or generally encoded in the same way as other payloads), but dragging Protobuf into this code looks like a mistake.
i'm indifferent. let's look into protobuffing it once it's clear what we want to do. I still hope there is a way to avoid this PR entirely.
There was a problem hiding this comment.
I still hope there is a way to avoid this PR entirely.
Agreed.
neongreen
dismissed
their stale review
Dismissing my review because there's still a future integration test to be reviewed.
fisx
force-pushed
the
fisx-app-level-websocket-pongs
branch
from
b9c99a4
to
a8fd6ba
Compare
fisx
force-pushed
the
fisx-app-level-websocket-pongs
branch
from
a8fd6ba
to
17c189a
Compare
|
could not reproduce. perhaps it's related to #573 (comment) ? |
| WS.receiveData conn >>= atomically . writeTChan chread | ||
| h2 <- async . forever $ do | ||
| WS.sendTextData conn =<< atomically (readTChan chwrite) | ||
| wait h1 >> wait h2 -- one way of saying "don't die!" |
There was a problem hiding this comment.
Seems nicer to use concurrently_:
wsReaderWriter =
concurrently_
(forever $ WS.receiveData conn >>= atomically . writeTChan chread)
(forever $ WS.sendTextData conn =<< atomically (readTChan chwrite))
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)
Fixes #560.
@marcoconti83 this needs to be ok'ed by all clients, since it generates web socket messages that need to be handled by the clients. More complicated solutions are conceivable, like a way for the client to turn this on or off (but I'm not in favour of that).
I also wonder if there is really no other way for client to tell whether the browser has lost the websocket connection, but I am ready to believe that's the case.
tasks: