From 5dc51fbe50e632d5db225a2f6cbaaba1700e976c Mon Sep 17 00:00:00 2001 From: "jiayl@webrtc.org" Date: Thu, 29 May 2014 15:33:54 +0000 Subject: [PATCH] Closes the DataChannel when the send buffer is full or on transport errors. As stated in the spec. BUG=2645 R=pthatcher@google.com, wu@webrtc.org Review URL: https://webrtc-codereview.appspot.com/12619004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6270 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/datachannel.cc | 25 ++++++---- talk/app/webrtc/datachannel_unittest.cc | 47 +++++++++++++++++-- .../app/webrtc/test/fakedatachannelprovider.h | 14 +++++- talk/libjingle_tests.gyp | 2 +- 4 files changed, 71 insertions(+), 17 deletions(-) diff --git a/talk/app/webrtc/datachannel.cc b/talk/app/webrtc/datachannel.cc index 048e89a505..ef202692fd 100644 --- a/talk/app/webrtc/datachannel.cc +++ b/talk/app/webrtc/datachannel.cc @@ -164,17 +164,23 @@ bool DataChannel::Send(const DataBuffer& buffer) { // If the queue is non-empty, we're waiting for SignalReadyToSend, // so just add to the end of the queue and keep waiting. if (!queued_send_data_.empty()) { - return QueueSendData(buffer); + if (!QueueSendData(buffer)) { + if (data_channel_type_ == cricket::DCT_RTP) { + return false; + } + Close(); + } + return true; } cricket::SendDataResult send_result; if (!InternalSendWithoutQueueing(buffer, &send_result)) { - if (send_result == cricket::SDR_BLOCK) { - return QueueSendData(buffer); + if (data_channel_type_ == cricket::DCT_RTP) { + return false; + } + if (send_result != cricket::SDR_BLOCK || !QueueSendData(buffer)) { + Close(); } - // Fail for other results. - // TODO(jiayl): We should close the data channel in this case. - return false; } return true; } @@ -325,9 +331,8 @@ void DataChannel::OnDataReceived(cricket::DataChannel* channel, observer_->OnMessage(*buffer.get()); } else { if (queued_received_data_.size() > kMaxQueuedReceivedDataPackets) { - // TODO(jiayl): We should close the data channel in this case. LOG(LS_ERROR) - << "Queued received data exceeds the max number of packes."; + << "Queued received data exceeds the max number of packets."; ClearQueuedReceivedData(); } queued_received_data_.push(buffer.release()); @@ -522,8 +527,8 @@ bool DataChannel::InternalSendWithoutQueueing( } bool DataChannel::QueueSendData(const DataBuffer& buffer) { - if (queued_send_data_.size() > kMaxQueuedSendDataPackets) { - LOG(LS_ERROR) << "Can't buffer any more data in the data channel."; + if (queued_send_data_.size() >= kMaxQueuedSendDataPackets) { + LOG(LS_ERROR) << "Can't buffer any more data for the data channel."; return false; } queued_send_data_.push_back(new DataBuffer(buffer)); diff --git a/talk/app/webrtc/datachannel_unittest.cc b/talk/app/webrtc/datachannel_unittest.cc index 1be24e96ce..e132fb14d3 100644 --- a/talk/app/webrtc/datachannel_unittest.cc +++ b/talk/app/webrtc/datachannel_unittest.cc @@ -29,14 +29,24 @@ #include "talk/app/webrtc/sctputils.h" #include "talk/app/webrtc/test/fakedatachannelprovider.h" #include "talk/base/gunit.h" -#include "testing/base/public/gmock.h" using webrtc::DataChannel; class FakeDataChannelObserver : public webrtc::DataChannelObserver { public: - MOCK_METHOD0(OnStateChange, void()); - MOCK_METHOD1(OnMessage, void(const webrtc::DataBuffer& buffer)); + FakeDataChannelObserver() : messages_received_(0) {} + + void OnStateChange() {} + void OnMessage(const webrtc::DataBuffer& buffer) { + ++messages_received_; + } + + size_t messages_received() const { + return messages_received_; + } + + private: + size_t messages_received_; }; class SctpDataChannelTest : public testing::Test { @@ -233,12 +243,13 @@ TEST_F(SctpDataChannelTest, ReceiveDataWithInvalidSsrc) { SetChannelReady(); AddObserver(); - EXPECT_CALL(*(observer_.get()), OnMessage(testing::_)).Times(0); cricket::ReceiveDataParams params; params.ssrc = 0; webrtc::DataBuffer buffer("abcd"); webrtc_data_channel_->OnDataReceived(NULL, params, buffer.data); + + EXPECT_EQ(0U, observer_->messages_received()); } // Tests that the incoming messages with right ssrcs are acceted. @@ -247,13 +258,13 @@ TEST_F(SctpDataChannelTest, ReceiveDataWithValidSsrc) { SetChannelReady(); AddObserver(); - EXPECT_CALL(*(observer_.get()), OnMessage(testing::_)).Times(1); cricket::ReceiveDataParams params; params.ssrc = 1; webrtc::DataBuffer buffer("abcd"); webrtc_data_channel_->OnDataReceived(NULL, params, buffer.data); + EXPECT_EQ(1U, observer_->messages_received()); } // Tests that no CONTROL message is sent if the datachannel is negotiated and @@ -302,3 +313,29 @@ TEST_F(SctpDataChannelTest, OpenAckRoleInitialization) { webrtc::InternalDataChannelInit init2(base); EXPECT_EQ(webrtc::InternalDataChannelInit::kNone, init2.open_handshake_role); } + +// Tests that the DataChannel is closed if the sending buffer is full. +TEST_F(SctpDataChannelTest, ClosedWhenSendBufferFull) { + SetChannelReady(); + webrtc::DataBuffer buffer("abcd"); + provider_.set_send_blocked(true); + + for (size_t i = 0; i < 101; ++i) { + EXPECT_TRUE(webrtc_data_channel_->Send(buffer)); + } + + EXPECT_EQ(webrtc::DataChannelInterface::kClosed, + webrtc_data_channel_->state()); +} + +// Tests that the DataChannel is closed on transport errors. +TEST_F(SctpDataChannelTest, ClosedOnTransportError) { + SetChannelReady(); + webrtc::DataBuffer buffer("abcd"); + provider_.set_transport_error(); + + EXPECT_TRUE(webrtc_data_channel_->Send(buffer)); + + EXPECT_EQ(webrtc::DataChannelInterface::kClosed, + webrtc_data_channel_->state()); +} diff --git a/talk/app/webrtc/test/fakedatachannelprovider.h b/talk/app/webrtc/test/fakedatachannelprovider.h index 5b196983cd..053b3acce4 100644 --- a/talk/app/webrtc/test/fakedatachannelprovider.h +++ b/talk/app/webrtc/test/fakedatachannelprovider.h @@ -32,7 +32,8 @@ class FakeDataChannelProvider : public webrtc::DataChannelProviderInterface { FakeDataChannelProvider() : send_blocked_(false), transport_available_(false), - ready_to_send_(false) {} + ready_to_send_(false), + transport_error_(false) {} virtual ~FakeDataChannelProvider() {} virtual bool SendData(const cricket::SendDataParams& params, @@ -43,6 +44,12 @@ class FakeDataChannelProvider : public webrtc::DataChannelProviderInterface { *result = cricket::SDR_BLOCK; return false; } + + if (transport_error_) { + *result = cricket::SDR_ERROR; + return false; + } + last_send_data_params_ = params; return true; } @@ -115,6 +122,10 @@ class FakeDataChannelProvider : public webrtc::DataChannelProviderInterface { } } + void set_transport_error() { + transport_error_ = true; + } + cricket::SendDataParams last_send_data_params() const { return last_send_data_params_; } @@ -136,6 +147,7 @@ class FakeDataChannelProvider : public webrtc::DataChannelProviderInterface { bool send_blocked_; bool transport_available_; bool ready_to_send_; + bool transport_error_; std::set connected_channels_; std::set send_ssrcs_; std::set recv_ssrcs_; diff --git a/talk/libjingle_tests.gyp b/talk/libjingle_tests.gyp index d21c77549d..1cbfe85ddf 100755 --- a/talk/libjingle_tests.gyp +++ b/talk/libjingle_tests.gyp @@ -393,7 +393,7 @@ ], # TODO(ronghuawu): Reenable below unit tests that require gmock. 'sources': [ - # 'app/webrtc/datachannel_unittest.cc', + 'app/webrtc/datachannel_unittest.cc', 'app/webrtc/dtmfsender_unittest.cc', 'app/webrtc/jsepsessiondescription_unittest.cc', 'app/webrtc/localaudiosource_unittest.cc',