From f8063d34deefb55b4a0e5091fc59d5d5e58e43d8 Mon Sep 17 00:00:00 2001 From: "jiayl@webrtc.org" Date: Wed, 18 Jun 2014 21:30:40 +0000 Subject: [PATCH] Properly shut down the SCTP stack. TBR phoglund@webrtc.org for the tsan_v2/suppressions.txt change. R=ldixon@webrtc.org, pthatcher@webrtc.org TBR=phoglund@webrtc.org BUG=2749 Review URL: https://webrtc-codereview.appspot.com/12739004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6484 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../src/org/webrtc/PeerConnectionTest.java | 9 +++---- talk/libjingle_tests.gyp | 4 +-- talk/media/sctp/sctpdataengine.cc | 26 ++++++++++--------- talk/media/sctp/sctpdataengine_unittest.cc | 20 +++++++------- ...ibjingle_media_unittest.gtest-memcheck.txt | 7 +++-- .../valgrind-webrtc/tsan_v2/suppressions.txt | 4 +++ 6 files changed, 37 insertions(+), 33 deletions(-) diff --git a/talk/app/webrtc/javatests/src/org/webrtc/PeerConnectionTest.java b/talk/app/webrtc/javatests/src/org/webrtc/PeerConnectionTest.java index b171b5841..240e996bd 100644 --- a/talk/app/webrtc/javatests/src/org/webrtc/PeerConnectionTest.java +++ b/talk/app/webrtc/javatests/src/org/webrtc/PeerConnectionTest.java @@ -525,7 +525,7 @@ public class PeerConnectionTest extends TestCase { private void doTest() throws Exception { CountDownLatch testDone = new CountDownLatch(1); System.gc(); // Encourage any GC-related threads to start up. - //TreeSet threadsBeforeTest = allThreads(); + TreeSet threadsBeforeTest = allThreads(); PeerConnectionFactory factory = new PeerConnectionFactory(); // Uncomment to get ALL WebRTC tracing and SENSITIVE libjingle logging. @@ -742,11 +742,8 @@ public class PeerConnectionTest extends TestCase { factory.dispose(); System.gc(); - // TODO(ldixon): the usrsctp threads are not cleaned up (issue 2749) and - // caused the assert to fail. We should reenable the assert once issue 2749 - // is fixed. - //TreeSet threadsAfterTest = allThreads(); - //assertEquals(threadsBeforeTest, threadsAfterTest); + TreeSet threadsAfterTest = allThreads(); + assertEquals(threadsBeforeTest, threadsAfterTest); Thread.sleep(100); } diff --git a/talk/libjingle_tests.gyp b/talk/libjingle_tests.gyp index 00e2230f5..016f0a506 100755 --- a/talk/libjingle_tests.gyp +++ b/talk/libjingle_tests.gyp @@ -287,9 +287,7 @@ 'media/base/videoengine_unittest.h', 'media/devices/dummydevicemanager_unittest.cc', 'media/devices/filevideocapturer_unittest.cc', - # TODO(jiayl): Enable the SCTP test once the memcheck and tsan bots - # failures are fixed (issue 2846). - #'media/sctp/sctpdataengine_unittest.cc', + 'media/sctp/sctpdataengine_unittest.cc', 'media/webrtc/webrtcpassthroughrender_unittest.cc', 'media/webrtc/webrtcvideocapturer_unittest.cc', # Omitted because depends on non-open-source testdata files. diff --git a/talk/media/sctp/sctpdataengine.cc b/talk/media/sctp/sctpdataengine.cc index 46b2ece43..3647d2129 100644 --- a/talk/media/sctp/sctpdataengine.cc +++ b/talk/media/sctp/sctpdataengine.cc @@ -277,18 +277,20 @@ SctpDataEngine::SctpDataEngine() { } SctpDataEngine::~SctpDataEngine() { - // TODO(ldixon): There is currently a bug in teardown of usrsctp that blocks - // indefintely if a finish call made too soon after close calls. So teardown - // has been skipped. Once the bug is fixed, retest and enable teardown. - // Tracked in webrtc issue 2749. - // - // usrsctp_engines_count--; - // LOG(LS_VERBOSE) << "usrsctp_engines_count:" << usrsctp_engines_count; - // if (usrsctp_engines_count == 0) { - // if (usrsctp_finish() != 0) { - // LOG(LS_WARNING) << "usrsctp_finish."; - // } - // } + usrsctp_engines_count--; + LOG(LS_VERBOSE) << "usrsctp_engines_count:" << usrsctp_engines_count; + + if (usrsctp_engines_count == 0) { + // usrsctp_finish() may fail if it's called too soon after the channels are + // closed. Wait and try again until it succeeds for up to 3 seconds. + for (size_t i = 0; i < 300; ++i) { + if (usrsctp_finish() == 0) + return; + + talk_base::Thread::SleepMs(10); + } + LOG(LS_ERROR) << "Failed to shutdown usrsctp."; + } } DataMediaChannel* SctpDataEngine::CreateChannel( diff --git a/talk/media/sctp/sctpdataengine_unittest.cc b/talk/media/sctp/sctpdataengine_unittest.cc index 092524b4c..ce6f80a50 100644 --- a/talk/media/sctp/sctpdataengine_unittest.cc +++ b/talk/media/sctp/sctpdataengine_unittest.cc @@ -295,7 +295,7 @@ class SctpDataMediaChannelTest : public testing::Test, params.ssrc = ssrc; return chan->SendData(params, talk_base::Buffer( - msg.data(), msg.length()), result); + &msg[0], msg.length()), result); } bool ReceivedData(const SctpFakeDataReceiver* recv, uint32 ssrc, @@ -364,26 +364,26 @@ TEST_F(SctpDataMediaChannelTest, SendData) { EXPECT_EQ(cricket::SDR_SUCCESS, result); EXPECT_TRUE_WAIT(ReceivedData(receiver2(), 1, "hello?"), 1000); LOG(LS_VERBOSE) << "recv2.received=" << receiver2()->received() - << "recv2.last_params.ssrc=" + << ", recv2.last_params.ssrc=" << receiver2()->last_params().ssrc - << "recv2.last_params.timestamp=" + << ", recv2.last_params.timestamp=" << receiver2()->last_params().ssrc - << "recv2.last_params.seq_num=" + << ", recv2.last_params.seq_num=" << receiver2()->last_params().seq_num - << "recv2.last_data=" << receiver2()->last_data(); + << ", recv2.last_data=" << receiver2()->last_data(); LOG(LS_VERBOSE) << "chan2 sending: 'hi chan1' -----------------------------"; ASSERT_TRUE(SendData(channel2(), 2, "hi chan1", &result)); EXPECT_EQ(cricket::SDR_SUCCESS, result); EXPECT_TRUE_WAIT(ReceivedData(receiver1(), 2, "hi chan1"), 1000); LOG(LS_VERBOSE) << "recv1.received=" << receiver1()->received() - << "recv1.last_params.ssrc=" + << ", recv1.last_params.ssrc=" << receiver1()->last_params().ssrc - << "recv1.last_params.timestamp=" + << ", recv1.last_params.timestamp=" << receiver1()->last_params().ssrc - << "recv1.last_params.seq_num=" + << ", recv1.last_params.seq_num=" << receiver1()->last_params().seq_num - << "recv1.last_data=" << receiver1()->last_data(); + << ", recv1.last_data=" << receiver1()->last_data(); } // Sends a lot of large messages at once and verifies SDR_BLOCK is returned. @@ -398,7 +398,7 @@ TEST_F(SctpDataMediaChannelTest, SendDataBlocked) { for (size_t i = 0; i < 100; ++i) { channel1()->SendData( - params, talk_base::Buffer(buffer.data(), buffer.size()), &result); + params, talk_base::Buffer(&buffer[0], buffer.size()), &result); if (result == cricket::SDR_BLOCK) break; } diff --git a/tools/valgrind-webrtc/gtest_exclude/libjingle_media_unittest.gtest-memcheck.txt b/tools/valgrind-webrtc/gtest_exclude/libjingle_media_unittest.gtest-memcheck.txt index 4727c59eb..9309510a7 100644 --- a/tools/valgrind-webrtc/gtest_exclude/libjingle_media_unittest.gtest-memcheck.txt +++ b/tools/valgrind-webrtc/gtest_exclude/libjingle_media_unittest.gtest-memcheck.txt @@ -1,2 +1,5 @@ -TODO(wu): https://code.google.com/p/webrtc/issues/detail?id=2380 -WebRtcVideoMediaChannelTest.TwoStreamsSendAndUnsignalledRecv \ No newline at end of file +#TODO(wu): https://code.google.com/p/webrtc/issues/detail?id=2380 +WebRtcVideoMediaChannelTest.TwoStreamsSendAndUnsignalledRecv + +#TODO(jiayl): https://code.google.com/p/webrtc/issues/detail?id=3492 +SctpDataMediaChannelTest.* diff --git a/tools/valgrind-webrtc/tsan_v2/suppressions.txt b/tools/valgrind-webrtc/tsan_v2/suppressions.txt index 5dca18475..2591157b9 100644 --- a/tools/valgrind-webrtc/tsan_v2/suppressions.txt +++ b/tools/valgrind-webrtc/tsan_v2/suppressions.txt @@ -31,3 +31,7 @@ race:talk/base/logging.cc race:talk/base/sharedexclusivelock_unittest.cc race:talk/base/signalthread_unittest.cc race:talk/base/thread.cc + +# third_party/usrsctp +# TODO(jiayl): https://code.google.com/p/webrtc/issues/detail?id=3492 +race:third_party/usrsctp/usrsctplib/user_sctp_timer_iterate.c