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

Better pings 2 #84

Merged
merged 2 commits into from Oct 4, 2012
Merged

Better pings 2 #84

merged 2 commits into from Oct 4, 2012

Conversation

cadorn
Copy link
Contributor

@cadorn cadorn commented Sep 12, 2012

This pull-request cleans up the commits for #78.
For previous discussion see: #78

@cadorn
Copy link
Contributor Author

cadorn commented Sep 12, 2012

Also, since you're now sending the pingInterval in the handshake we need to update SPEC in the engine.io-client repo and bump up the revision to 2, since clients will be rendered incompatible.

Thinking about this reminded me of an observation I made when writing this patch.

Why are pings initiated by the server?

This patch provides the following:

server on `pingInterval` send `ping` and expect `pong` within `pingTimeout` or emit `ping timeout`
client on `ping` send `pong` and expect ack `pong` within `pingTimeout` or emit `ping timeout`
client expect `ping` within every `pingInterval + pingTimeout` or emit `ping timeout`

By necessity to accomplish pingInterval + pingTimeout timeout on client, server must send an ack pong packet (this is new). Instead of the ack pong packet we cloud look for clean transport request close after sending pong packet on client but that does not ensure server is getting our responses.

If pings were initiated by client we could get rid of the ack pong packet (good especially for polling) and still timeout within pingInterval + pingTimeout on client:

client on `pingInterval` send `ping` and expect `pong` within `pingTimeout` or emit `ping timeout`
server on `ping` send `pong` and expect next `ping` within `pingInterval + pingTimeout`

This would mean more changes to the SPEC and client implementations but is a cleaner and lighter lifecycle. I am leaning towards this approach as I typically always initiate pings on clients.

@cadorn cadorn mentioned this pull request Sep 12, 2012
@rauchg
Copy link
Contributor

rauchg commented Sep 12, 2012

Sounds good, since we're changing pings we might as well switch to that. Good point @cadorn

@cadorn
Copy link
Contributor Author

cadorn commented Sep 12, 2012

Done here & socketio/engine.io-client#51

All tests still pass! (other than two minor adjustments)

Now only need to update SPEC.

rauchg added a commit that referenced this pull request Oct 4, 2012
@rauchg rauchg merged commit 2f806d3 into socketio:master Oct 4, 2012
@rauchg
Copy link
Contributor

rauchg commented Oct 4, 2012

@cadorn just merged this, looking good. Make sure to submit a PR for SPEC.

@rauchg
Copy link
Contributor

rauchg commented Oct 4, 2012

Thanks a lot!

@cadorn
Copy link
Contributor Author

cadorn commented Nov 5, 2012

See #115 for spec.

darrachequesne pushed a commit that referenced this pull request May 8, 2020
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

Successfully merging this pull request may close these issues.

None yet

2 participants