Implement heartbeat functionality #224

futile · 2019-08-04T12:56:14Z

Make sure that connections don't time out if we don't want them to, by
regularly sending special, empty "heartbeat" packets on connections that we
have not sent on for some time.

These heartbeats have the new packet type PacketType::Heartbeat, which makes it possible to differentiate them from empty packets sent by a user.

The heartbeat interval can be configured and disabled/enabled via Config::heartbeat_interval.

~~Note: Currently these heartbeats can not be explicity disabled (but you can set a very long heartbeat interval), but that should not be hard to add, if desired.~~ Edit: Heartbeats can now be disabled.

codecov · 2019-08-04T13:05:44Z

Codecov Report

Merging #224 into master will decrease coverage by 0.03%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
- Coverage   97.49%   97.45%   -0.04%     
==========================================
  Files          25       25              
  Lines        2357     2439      +82     
==========================================
+ Hits         2298     2377      +79     
- Misses         59       62       +3

Impacted Files	Coverage Δ
src/packet/enums.rs	`97.1% <100%> (+0.17%)`	⬆️
src/packet/header/standard_header.rs	`98.64% <100%> (+0.05%)`	⬆️
src/net/virtual_connection.rs	`96.91% <100%> (+0.1%)`	⬆️
src/net/connection.rs	`100% <100%> (ø)`	⬆️
src/config.rs	`100% <100%> (ø)`	⬆️
src/net/socket.rs	`96.05% <95.58%> (-0.18%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d8c65f...74dd68f. Read the comment docs.

futile · 2019-08-04T13:20:28Z

If I understand the codecov report correctly, everything except for errors that can be returned from Socket::send_to is covered. Any ideas on how to test these remaining cases? The same kind of error is also returned by Socket::handle_idle_clients, but it's also not tested there, so I can't test it the same way.

TimonPost · 2019-08-04T13:21:04Z

src/config.rs

@@ -52,6 +54,7 @@ impl Default for Config {
        Self {
            blocking_mode: false,
            idle_connection_timeout: Duration::from_secs(5),
+            heartbeat_interval: Duration::from_secs(2),


I think we should probably, in the future, have this calculated based on the current network circumstances. With two seconds this is safe, we might as well move it up to 3, because the idling time is 5 seconds, then the heartbeat has a total time of 2 seconds to arrive at the client.

Yes, I thought about that as well, but didn't want to add RTT calculations into it (for now). Although I agree that that is probably a good idea here.

I chose 2 seconds because then if one heartbeat packet is lost, we have enough time (probably) to send another one before the connection times out. Then again, this really depends on the idle_connection_timeout value of the receiver, so the user currently has to adjust this value to their wishes anyway.

Right, I can understand that I am not too certain about our RTT calculations as well, so I am fine with having this statically typed now. And figure out a way to do it based on the current internet speed in the future.

TimonPost · 2019-08-04T13:22:25Z

src/net/socket.rs

        if let Err(e) = self.handle_idle_clients(time) {
            error!("Encountered an error when sending TimeoutEvent: {:?}", e);
        }
+
+        // Finally send heartbeat packets to connections that require them
+        if let Err(e) = self.send_heartbeat_packets(time) {


As discussed, I prefer to have this optional.

Sure, should it be enabled or disabled by default?

Made heartbeats disabled by default, and the config value is now an Option<Duration>, where None disables and Some(Duration) enables heartbeats.

TimonPost · 2019-08-04T13:31:45Z

Thanks for the PR, I do have one question are the acknowledgments also sent when the user only sents unreliable packets, because I don't see much-added value to have a heartbeat for those guarantees.

futile · 2019-08-04T13:48:35Z

I think the CI failure is spurious. I'm also working from windows and also saw it once (for no reason). Also I think something similar happened during my previous documentation-PR?

futile · 2019-08-04T14:02:48Z

Thanks for the PR, I do have one question are the acknowledgments also sent when the user only sents unreliable packets, because I don't see much-added value to have a heartbeat for those guarantees.

I'm not quite sure what you mean, but heartbeats are currently send as unreliable and unordered, so there are no acknowledgments sent out for heartbeats atm. Did I understand your question correctly?

Make sure that connections don't time out if we don't want them to, by regularly sending special, empty "heartbeat" packets on connections that we have not sent on for some time. The heartbeat interval can be configured and enabled/disabled via `Config::heartbeat_interval'.

TimonPost · 2019-08-04T21:22:15Z

I meant more in the way of, when sending packets, we can do use unreliably unordered to so for example. For unreliable packets, we do not have any internal state as we have for reliable ordered. So we might not care about the client to disconnect, on the other hand, I do think now, that you might also want to keep track of connected clients when only sending unreliable packets. So is not a direct problem, as long the heartbeat is optional im OK with it

jstnlef

This looks good to me 👍.

jstnlef · 2019-08-12T03:16:55Z

@TimonPost I believe that maintaining connection is an orthogonal concern to whether or not reliable packets are sent and having the option to ensure heartbeats are sent when a client is idling seems reasonable to me.

futile · 2019-08-12T08:20:54Z

Thanks for the review! :)

TimonPost · 2019-08-12T16:59:17Z

Agreed, let's merge this!

TimonPost · 2019-08-12T16:59:58Z

bors r=TimonPost, jstnlef

224: Implement heartbeat functionality r=TimonPost,jstnlef a=futile Make sure that connections don't time out if we don't want them to, by regularly sending special, empty "heartbeat" packets on connections that we have not sent on for some time. These heartbeats have the new packet type `PacketType::Heartbeat`, which makes it possible to differentiate them from empty packets sent by a user. The heartbeat interval can be configured and disabled/enabled via `Config::heartbeat_interval`. ~~Note: Currently these heartbeats can not be explicity disabled (but you can set a very long heartbeat interval), but that should not be hard to add, if desired.~~ Edit: Heartbeats can now be disabled. Co-authored-by: Felix Rath <felix.rath@rwth-aachen.de>

bors · 2019-08-12T18:29:58Z

Timed out

TimonPost requested a review from jstnlef August 4, 2019 12:59

futile force-pushed the heartbeats branch from 5e1a035 to 0c0c02e Compare August 4, 2019 13:00

TimonPost reviewed Aug 4, 2019

View reviewed changes

futile force-pushed the heartbeats branch from 0c0c02e to 6d001f9 Compare August 4, 2019 13:43

futile force-pushed the heartbeats branch from 6d001f9 to 74dd68f Compare August 4, 2019 14:08

jstnlef approved these changes Aug 12, 2019

View reviewed changes

TimonPost approved these changes Aug 12, 2019

View reviewed changes

TimonPost merged commit 58630ed into TimonPost:master Aug 12, 2019

jstnlef pushed a commit to jstnlef/laminar that referenced this pull request Oct 12, 2019

Implement heartbeat functionality (TimonPost#224)

f1f86b8

jstnlef pushed a commit that referenced this pull request Oct 12, 2019

Implement heartbeat functionality (#224)

f261269

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement heartbeat functionality #224

Implement heartbeat functionality #224

futile commented Aug 4, 2019 •

edited

Loading

codecov bot commented Aug 4, 2019 •

edited

Loading

futile commented Aug 4, 2019 •

edited

Loading

TimonPost Aug 4, 2019

futile Aug 4, 2019

TimonPost Aug 4, 2019

TimonPost Aug 4, 2019

futile Aug 4, 2019

futile Aug 4, 2019

TimonPost commented Aug 4, 2019

futile commented Aug 4, 2019 •

edited

Loading

futile commented Aug 4, 2019

TimonPost commented Aug 4, 2019

jstnlef left a comment

jstnlef commented Aug 12, 2019

futile commented Aug 12, 2019

TimonPost commented Aug 12, 2019

TimonPost commented Aug 12, 2019

bors bot commented Aug 12, 2019

Implement heartbeat functionality #224

Implement heartbeat functionality #224

Conversation

futile commented Aug 4, 2019 • edited Loading

codecov bot commented Aug 4, 2019 • edited Loading

Codecov Report

futile commented Aug 4, 2019 • edited Loading

TimonPost Aug 4, 2019

Choose a reason for hiding this comment

futile Aug 4, 2019

Choose a reason for hiding this comment

TimonPost Aug 4, 2019

Choose a reason for hiding this comment

TimonPost Aug 4, 2019

Choose a reason for hiding this comment

futile Aug 4, 2019

Choose a reason for hiding this comment

futile Aug 4, 2019

Choose a reason for hiding this comment

TimonPost commented Aug 4, 2019

futile commented Aug 4, 2019 • edited Loading

futile commented Aug 4, 2019

TimonPost commented Aug 4, 2019

jstnlef left a comment

Choose a reason for hiding this comment

jstnlef commented Aug 12, 2019

futile commented Aug 12, 2019

TimonPost commented Aug 12, 2019

TimonPost commented Aug 12, 2019

bors bot commented Aug 12, 2019

Timed out

futile commented Aug 4, 2019 •

edited

Loading

codecov bot commented Aug 4, 2019 •

edited

Loading

futile commented Aug 4, 2019 •

edited

Loading

futile commented Aug 4, 2019 •

edited

Loading