dcsctp: Restart heartbeat timer when sending DATA
Before this change, the heartbeat timer was restarted every time a
packet was sent on the socket. On an idle connection, if the peer is
sending heartbeats, just responding to those heartbeats (with a
HEARTBEAT-ACK) would restart the timer, and then this socket wouldn't
do any heartbeating itself because the next hearbeat by the peer would
be received before the timer expires.
This is not according to the specification, where
https://datatracker.ietf.org/doc/html/rfc9260#section-8.3 states that
"A destination transport address is considered "idle" if no new chunk
that can be used for updating path RTT (usually including first
transmission DATA, INIT, COOKIE ECHO, or HEARTBEAT chunks, etc.)"
There are already timers running when INIT, and COOKIE-ECHO are sent
and not acked, so the heartbeat shouldn't be sent then. This is further
confirmed in the same section in the RFC which says that "The sending of
HEARTBEAT chunks MAY begin upon reaching the ESTABLISHED state". And
when INIT and COOKIE-ECHO are sent, the connection is not yet
established.
This CL changes so that the heartbeat timer is only restarted when any
DATA or I-DATA chunk is sent. This will make both sides send heartbeats
on an idle connection.
Bug: webrtc:343600379
Change-Id: I5ab159b7901e2ec9d37b24aaf845891b60a53c13
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/352841
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42409}
diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc
index d197a38..48b6edb 100644
--- a/net/dcsctp/socket/dcsctp_socket.cc
+++ b/net/dcsctp/socket/dcsctp_socket.cc
@@ -1021,12 +1021,6 @@
DebugPrintOutgoing(packet);
}
- // The heartbeat interval timer is restarted for every sent packet, to
- // fire when the outgoing channel is inactive.
- if (tcb_ != nullptr) {
- tcb_->heartbeat_handler().RestartTimer();
- }
-
++metrics_.tx_packets_count;
}
}
diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc
index 2d392d6..d27b260 100644
--- a/net/dcsctp/socket/dcsctp_socket_test.cc
+++ b/net/dcsctp/socket/dcsctp_socket_test.cc
@@ -910,6 +910,37 @@
MaybeHandoverSocketAndSendMessage(a, std::move(z));
}
+TEST(DcSctpSocketParametrizedTest, BothSidesSendHeartbeats) {
+ // On an idle connection, both sides send heartbeats, and both sides acks.
+
+ // Make them have slightly different heartbeat intervals, to validate that
+ // sending an ack by Z doesn't restart its heartbeat timer.
+ DcSctpOptions options_a = {.heartbeat_interval = DurationMs(1000)};
+ SocketUnderTest a("A", options_a);
+
+ DcSctpOptions options_z = {.heartbeat_interval = DurationMs(1100)};
+ SocketUnderTest z("Z", options_z);
+
+ ConnectSockets(a, z);
+
+ AdvanceTime(a, z, TimeDelta::Millis(1000));
+
+ std::vector<uint8_t> packet_a = a.cb.ConsumeSentPacket();
+ EXPECT_THAT(packet_a, HasChunks(ElementsAre(IsHeartbeatRequest(_))));
+ // Z receives heartbeat, sends ACK.
+ z.socket.ReceivePacket(packet_a);
+ a.socket.ReceivePacket(z.cb.ConsumeSentPacket());
+
+ // A little while later, Z should send heartbeats to A.
+ AdvanceTime(a, z, TimeDelta::Millis(100));
+
+ std::vector<uint8_t> packet_z = z.cb.ConsumeSentPacket();
+ EXPECT_THAT(packet_z, HasChunks(ElementsAre(IsHeartbeatRequest(_))));
+ // A receives heartbeat, sends ACK.
+ a.socket.ReceivePacket(packet_z);
+ z.socket.ReceivePacket(a.cb.ConsumeSentPacket());
+}
+
TEST_P(DcSctpSocketParametrizedTest,
CloseConnectionAfterTooManyLostHeartbeats) {
SocketUnderTest a("A");
diff --git a/net/dcsctp/socket/transmission_control_block.cc b/net/dcsctp/socket/transmission_control_block.cc
index 3422341..e179a8e 100644
--- a/net/dcsctp/socket/transmission_control_block.cc
+++ b/net/dcsctp/socket/transmission_control_block.cc
@@ -244,6 +244,13 @@
auto chunks =
retransmission_queue_.GetChunksToSend(now, builder.bytes_remaining());
+
+ if (!chunks.empty()) {
+ // https://datatracker.ietf.org/doc/html/rfc9260#section-8.3
+ // Sending DATA means that the path is not idle - restart heartbeat timer.
+ heartbeat_handler_.RestartTimer();
+ }
+
for (auto& [tsn, data] : chunks) {
if (capabilities_.message_interleaving) {
builder.Add(IDataChunk(tsn, std::move(data), false));
