Fixed and improved timeout implementation for transports (heartbeat_interval and connection_close) #49

Closed
wants to merge 4 commits into
from

Projects

None yet

4 participants

@jannschu
Contributor
  • Reduced number of timer resets
  • Client heartbeats are now considered
  • Correct connection_close time is used

Currently only for xhr-multipart. I will add the others (wait before merge) and maybe also tests.
Please discuss.

@ferd

Is there any specific reason for suddenly adding this constraint there? Just making the invariants more obvious?

Owner

Yes there is a reason. After closing a tab with an open connection for some reason the last message was re-sent. This constraint avoids starting the heartbeat timeout (which resets the close_connection timeout).

@ferd

Any specific reason for removing that one?

Owner

Yes, of course.

In {initialize, ...}-cast an timer for a heartbeat is created. I for myself think it isn't that useful to send a heartbeat immediately after a connection is opened. So the first heartbeat is sent after a heartbeat_interval.

@ferd

you only need the catch because the timer reference is not reference. If you keep the default value I had of make_ref(), then you can just transparently call for a cancel_timer without needing any error checking, which makes it somewhat nicer to read IMO.

Owner

Thats a matter of taste I think. With the proposed solution you have to create a meaningless reference.

@ferd

I'm not sure I get why we have this clause here and why it leads to a shutdown. I might be thick because it's 7 in the morning, but care to explain?

Owner

If the client did not respond to the last heartbeat the connection should be considered to be closed. Or did I understand this one wrong?

ferd replied Jun 24, 2011

I'm checking in the node.js implementation and unless I don't get the code, there seems to be no check regarding whether the heartbeats are in right order or not. They seem to be used only to reset timers, and not as a consistency check.

Owner

Agreed. There seems to be no numbering at all, think a client id is used what here is index.

But: The current implementation is then buggy as well. First the index should stay the same. Secondly if the client does not response to a heartbeat the server just continues to send 'heartbeat requests' (see timer reset in the clause). So to fix it, the heartbeat clause must check when the last msg/heartbeat from the client received.

@omarkj
Collaborator
omarkj commented Jun 18, 2011

Just went through this and am okay with it - haven't tried it but it looks okay. @yrashk, what do you think? I think we can pull this in.

@jannschu
Contributor

Maybe wait until I changed htmlfile as well. But can't test it here.

@jannschu
Contributor

So, htmlfile changed. But it is untested.

@yrashk
Owner
yrashk commented Jul 23, 2011

@omarkj have you tried to test this pull request yet?

@omarkj
Collaborator
omarkj commented Jul 23, 2011

No. I'll try it.

On 23.7.2011, at 09:12, yrashk
reply@reply.github.com
wrote:

@omarkj have you tried to test this pull request yet?

Reply to this email directly or view it on GitHub:
#49 (comment)

@omarkj
Collaborator
omarkj commented Oct 14, 2015

Closing this.

@omarkj omarkj closed this Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment