Skip to content

Commit

Permalink
Propagate socket write errors for DtlsTransport
Browse files Browse the repository at this point in the history
The UDP sockets in WebRTC are non-blocking, and when writing too much
to them so that their send buffer becomes exhausted, they will return
EAGAIN or EWOULDBLOCK, which indicates that the client will need to
retry a bit later.

Media packets are generally sent by the pacer, which generally avoids
this exhaustion, but for SCTP which has a congestion control algorithm
quite similar to TCP, it may overshoot the amount of data it writes. If
the SCTP library can be notified when writing fails, it can stop writing
for a while until the socket recovers, which will result in less
overshooting and fewer lost packets (possibly even none). But for the
SCTP library to be able to know this, errors must be propagated, which
they weren't with the argument that packets may get lost anyway.

Bug: webrtc:12943
Change-Id: I9244580abf0d48ff810da30a23f995d12be623ed
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228439
Reviewed-by: Tommi <[email protected]>
Commit-Queue: Tommi <[email protected]>
Cr-Commit-Position: refs/heads/master@{#34751}
  • Loading branch information
Victor Boivie authored and WebRTC LUCI CQ committed Aug 13, 2021
1 parent 55a2f77 commit edfaaef
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 6 deletions.
15 changes: 10 additions & 5 deletions p2p/base/dtls_transport.cc
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,17 @@ rtc::StreamResult StreamInterfaceChannel::Write(const void* data,
size_t* written,
int* error) {
RTC_DCHECK_RUN_ON(&sequence_checker_);
// Always succeeds, since this is an unreliable transport anyway.
// TODO(zhihuang): Should this block if ice_transport_'s temporarily
// unwritable?
rtc::PacketOptions packet_options;
ice_transport_->SendPacket(static_cast<const char*>(data), data_len,
packet_options);
int sent = ice_transport_->SendPacket(static_cast<const char*>(data),
data_len, packet_options);
if (sent < 0) {
if (written) {
*written = 0;
}
return rtc::IsBlockingError(ice_transport_->GetError()) ? rtc::SR_BLOCK
: rtc::SR_ERROR;
}

if (written) {
*written = data_len;
}
Expand Down
10 changes: 10 additions & 0 deletions p2p/base/dtls_transport_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,16 @@ TEST_F(DtlsTransportTest, TestTransferDtls) {
TestTransfer(1000, 100, /*srtp=*/false);
}

// Connect with DTLS, and fail to write, to ensure errors are propagated.
TEST_F(DtlsTransportTest, TestWriteFailsOverDtls) {
PrepareDtls(rtc::KT_DEFAULT);
ASSERT_TRUE(Connect());
client1_.fake_ice_transport()->SetError(EAGAIN);
int res = client1_.dtls_transport()->SendPacket("hello", 5,
rtc::PacketOptions(), 0);
EXPECT_EQ(res, -1);
}

// Connect with DTLS, combine multiple DTLS records into one packet.
// Our DTLS implementation doesn't do this, but other implementations may;
// see https://tools.ietf.org/html/rfc6347#section-4.1.1.
Expand Down
17 changes: 16 additions & 1 deletion p2p/base/fake_ice_transport.h
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,10 @@ class FakeIceTransport : public IceTransportInternal {
return -1;
}

if (error_ != 0) {
return -1;
}

send_packet_.AppendData(data, len);
if (!combine_outgoing_packets_ || send_packet_.size() > len) {
rtc::CopyOnWriteBuffer packet(std::move(send_packet_));
Expand Down Expand Up @@ -338,7 +342,17 @@ class FakeIceTransport : public IceTransportInternal {
}
}

int GetError() override { return 0; }
// Sets the error that is returned by `GetError`. If an error is set (i.e.
// non-zero), `SendPacket` will return -1.
void SetError(int error) {
RTC_DCHECK_RUN_ON(network_thread_);
error_ = error;
}

int GetError() override {
RTC_DCHECK_RUN_ON(network_thread_);
return error_;
}

rtc::CopyOnWriteBuffer last_sent_packet() {
RTC_DCHECK_RUN_ON(network_thread_);
Expand Down Expand Up @@ -421,6 +435,7 @@ class FakeIceTransport : public IceTransportInternal {
rtc::CopyOnWriteBuffer last_sent_packet_ RTC_GUARDED_BY(network_thread_);
rtc::Thread* const network_thread_;
webrtc::ScopedTaskSafetyDetached task_safety_;
int error_ RTC_GUARDED_BY(network_thread_) = 0;
};

class FakeIceTransportWrapper : public webrtc::IceTransportInterface {
Expand Down

0 comments on commit edfaaef

Please sign in to comment.