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

all messages to the server should be an object #556

Merged
merged 2 commits into from Nov 15, 2016

Conversation

Projects
None yet
3 participants
@ibdknox
Contributor

ibdknox commented Nov 14, 2016

The "ping" message was causing the server to crash because it isn't an object.

all messages to the server should be an object
Signed-off-by: Chris Granger <ibdknox@gmail.com>
@joshuafcole

This comment has been minimized.

Contributor

joshuafcole commented Nov 14, 2016

Might be worth changing the throw to a console.error while we're at it.

@convolvatron
Contributor

convolvatron left a comment

seem legit. i really need to review all this. more broadly whats the rationale to run an additional ping at this level?

don't throw an error, but warn in the log when a malformed event come…
…s down the socket

Signed-off-by: Chris Granger <ibdknox@gmail.com>
@ibdknox

This comment has been minimized.

Contributor

ibdknox commented Nov 14, 2016

@convolvatron the ping is there because inactive sockets are closed after a period of inactivity on a lot of hosts (e.g. heroku). So we have to send a message to the server every once in awhile to make sure that the socket stays open even if the user isn't doing anything on the page at the time.

@joshuafcole

This comment has been minimized.

Contributor

joshuafcole commented Nov 14, 2016

I think this is a holdover from issues we had previously where the browser would occasionally kill websockets that had been quiet too long. I don't know that we ever figured out specifically why, but this was an empirical solution.

@ibdknox ibdknox merged commit a499a2d into master Nov 15, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@ibdknox ibdknox deleted the fix-ping branch Nov 15, 2016

@convolvatron

This comment has been minimized.

Contributor

convolvatron commented Nov 15, 2016

as usual i'm confused about the underlying truth, but i think that the ws protocol explicitly
send periodic web socket level pings, and if those aren't flowing then the remote peer or
the OS might decided to just give up. so it shouldn't(?) be necessary to have keepalives
above ws..but i don't know that, and its entirely possible that Heroku went to extreme effort
to try to filter out the ws level pings

@ibdknox

This comment has been minimized.

Contributor

ibdknox commented Nov 15, 2016

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