diff --git a/rtc_base/physical_socket_server.cc b/rtc_base/physical_socket_server.cc index 080534af2c..2d328dd350 100644 --- a/rtc_base/physical_socket_server.cc +++ b/rtc_base/physical_socket_server.cc @@ -24,7 +24,6 @@ // "poll" will be used to wait for the signal dispatcher. #include #endif -#include #include #include #include @@ -959,181 +958,6 @@ class EventDispatcher : public Dispatcher { CriticalSection crit_; }; -// These two classes use the self-pipe trick to deliver POSIX signals to our -// select loop. This is the only safe, reliable, cross-platform way to do -// non-trivial things with a POSIX signal in an event-driven program (until -// proper pselect() implementations become ubiquitous). - -class PosixSignalHandler { - public: - // POSIX only specifies 32 signals, but in principle the system might have - // more and the programmer might choose to use them, so we size our array - // for 128. - static constexpr int kNumPosixSignals = 128; - - // There is just a single global instance. (Signal handlers do not get any - // sort of user-defined void * parameter, so they can't access anything that - // isn't global.) - static PosixSignalHandler* Instance() { - static PosixSignalHandler* const instance = new PosixSignalHandler(); - return instance; - } - - // Returns true if the given signal number is set. - bool IsSignalSet(int signum) const { - RTC_DCHECK(signum < static_cast(arraysize(received_signal_))); - if (signum < static_cast(arraysize(received_signal_))) { - return received_signal_[signum]; - } else { - return false; - } - } - - // Clears the given signal number. - void ClearSignal(int signum) { - RTC_DCHECK(signum < static_cast(arraysize(received_signal_))); - if (signum < static_cast(arraysize(received_signal_))) { - received_signal_[signum] = false; - } - } - - // Returns the file descriptor to monitor for signal events. - int GetDescriptor() const { return afd_[0]; } - - // This is called directly from our real signal handler, so it must be - // signal-handler-safe. That means it cannot assume anything about the - // user-level state of the process, since the handler could be executed at any - // time on any thread. - void OnPosixSignalReceived(int signum) { - if (signum >= static_cast(arraysize(received_signal_))) { - // We don't have space in our array for this. - return; - } - // Set a flag saying we've seen this signal. - received_signal_[signum] = true; - // Notify application code that we got a signal. - const uint8_t b[1] = {0}; - if (-1 == write(afd_[1], b, sizeof(b))) { - // Nothing we can do here. If there's an error somehow then there's - // nothing we can safely do from a signal handler. - // No, we can't even safely log it. - // But, we still have to check the return value here. Otherwise, - // GCC 4.4.1 complains ignoring return value. Even (void) doesn't help. - return; - } - } - - private: - PosixSignalHandler() { - if (pipe(afd_) < 0) { - RTC_LOG_ERR(LS_ERROR) << "pipe failed"; - return; - } - if (fcntl(afd_[0], F_SETFL, O_NONBLOCK) < 0) { - RTC_LOG_ERR(LS_WARNING) << "fcntl #1 failed"; - } - if (fcntl(afd_[1], F_SETFL, O_NONBLOCK) < 0) { - RTC_LOG_ERR(LS_WARNING) << "fcntl #2 failed"; - } - memset(const_cast(static_cast(received_signal_)), 0, - sizeof(received_signal_)); - } - - ~PosixSignalHandler() { - int fd1 = afd_[0]; - int fd2 = afd_[1]; - // We clobber the stored file descriptor numbers here or else in principle - // a signal that happens to be delivered during application termination - // could erroneously write a zero byte to an unrelated file handle in - // OnPosixSignalReceived() if some other file happens to be opened later - // during shutdown and happens to be given the same file descriptor number - // as our pipe had. Unfortunately even with this precaution there is still a - // race where that could occur if said signal happens to be handled - // concurrently with this code and happens to have already read the value of - // afd_[1] from memory before we clobber it, but that's unlikely. - afd_[0] = -1; - afd_[1] = -1; - close(fd1); - close(fd2); - } - - int afd_[2]; - // These are boolean flags that will be set in our signal handler and read - // and cleared from Wait(). There is a race involved in this, but it is - // benign. The signal handler sets the flag before signaling the pipe, so - // we'll never end up blocking in select() while a flag is still true. - // However, if two of the same signal arrive close to each other then it's - // possible that the second time the handler may set the flag while it's still - // true, meaning that signal will be missed. But the first occurrence of it - // will still be handled, so this isn't a problem. - // Volatile is not necessary here for correctness, but this data _is_ volatile - // so I've marked it as such. - volatile uint8_t received_signal_[kNumPosixSignals]; -}; - -class PosixSignalDispatcher : public Dispatcher { - public: - PosixSignalDispatcher(PhysicalSocketServer* owner) : owner_(owner) { - owner_->Add(this); - } - - ~PosixSignalDispatcher() override { owner_->Remove(this); } - - uint32_t GetRequestedEvents() override { return DE_READ; } - - void OnPreEvent(uint32_t ff) override { - // Events might get grouped if signals come very fast, so we read out up to - // 16 bytes to make sure we keep the pipe empty. - uint8_t b[16]; - ssize_t ret = read(GetDescriptor(), b, sizeof(b)); - if (ret < 0) { - RTC_LOG_ERR(LS_WARNING) << "Error in read()"; - } else if (ret == 0) { - RTC_LOG(LS_WARNING) << "Should have read at least one byte"; - } - } - - void OnEvent(uint32_t ff, int err) override { - for (int signum = 0; signum < PosixSignalHandler::kNumPosixSignals; - ++signum) { - if (PosixSignalHandler::Instance()->IsSignalSet(signum)) { - PosixSignalHandler::Instance()->ClearSignal(signum); - HandlerMap::iterator i = handlers_.find(signum); - if (i == handlers_.end()) { - // This can happen if a signal is delivered to our process at around - // the same time as we unset our handler for it. It is not an error - // condition, but it's unusual enough to be worth logging. - RTC_LOG(LS_INFO) << "Received signal with no handler: " << signum; - } else { - // Otherwise, execute our handler. - (*i->second)(signum); - } - } - } - } - - int GetDescriptor() override { - return PosixSignalHandler::Instance()->GetDescriptor(); - } - - bool IsDescriptorClosed() override { return false; } - - void SetHandler(int signum, void (*handler)(int)) { - handlers_[signum] = handler; - } - - void ClearHandler(int signum) { handlers_.erase(signum); } - - bool HasHandlers() { return !handlers_.empty(); } - - private: - typedef std::map HandlerMap; - - HandlerMap handlers_; - // Our owner. - PhysicalSocketServer* owner_; -}; - #endif // WEBRTC_POSIX #if defined(WEBRTC_WIN) @@ -1226,9 +1050,6 @@ PhysicalSocketServer::PhysicalSocketServer() : fWait_(false) { PhysicalSocketServer::~PhysicalSocketServer() { #if defined(WEBRTC_WIN) WSACloseEvent(socket_ev_); -#endif -#if defined(WEBRTC_POSIX) - signal_dispatcher_.reset(); #endif delete signal_wakeup_; #if defined(WEBRTC_USE_EPOLL) @@ -1746,62 +1567,6 @@ bool PhysicalSocketServer::WaitPoll(int cmsWait, Dispatcher* dispatcher) { #endif // WEBRTC_USE_EPOLL -static void GlobalSignalHandler(int signum) { - PosixSignalHandler::Instance()->OnPosixSignalReceived(signum); -} - -bool PhysicalSocketServer::SetPosixSignalHandler(int signum, - void (*handler)(int)) { - // If handler is SIG_IGN or SIG_DFL then clear our user-level handler, - // otherwise set one. - if (handler == SIG_IGN || handler == SIG_DFL) { - if (!InstallSignal(signum, handler)) { - return false; - } - if (signal_dispatcher_) { - signal_dispatcher_->ClearHandler(signum); - if (!signal_dispatcher_->HasHandlers()) { - signal_dispatcher_.reset(); - } - } - } else { - if (!signal_dispatcher_) { - signal_dispatcher_.reset(new PosixSignalDispatcher(this)); - } - signal_dispatcher_->SetHandler(signum, handler); - if (!InstallSignal(signum, &GlobalSignalHandler)) { - return false; - } - } - return true; -} - -Dispatcher* PhysicalSocketServer::signal_dispatcher() { - return signal_dispatcher_.get(); -} - -bool PhysicalSocketServer::InstallSignal(int signum, void (*handler)(int)) { - struct sigaction act; - // It doesn't really matter what we set this mask to. - if (sigemptyset(&act.sa_mask) != 0) { - RTC_LOG_ERR(LS_ERROR) << "Couldn't set mask"; - return false; - } - act.sa_handler = handler; -#if !defined(__native_client__) - // Use SA_RESTART so that our syscalls don't get EINTR, since we don't need it - // and it's a nuisance. Though some syscalls still return EINTR and there's no - // real standard for which ones. :( - act.sa_flags = SA_RESTART; -#else - act.sa_flags = 0; -#endif - if (sigaction(signum, &act, nullptr) != 0) { - RTC_LOG_ERR(LS_ERROR) << "Couldn't set sigaction"; - return false; - } - return true; -} #endif // WEBRTC_POSIX #if defined(WEBRTC_WIN) diff --git a/rtc_base/physical_socket_server.h b/rtc_base/physical_socket_server.h index a71810f3db..05a8b1412b 100644 --- a/rtc_base/physical_socket_server.h +++ b/rtc_base/physical_socket_server.h @@ -41,9 +41,6 @@ enum DispatcherEvent { }; class Signaler; -#if defined(WEBRTC_POSIX) -class PosixSignalDispatcher; -#endif class Dispatcher { public: @@ -82,23 +79,6 @@ class RTC_EXPORT PhysicalSocketServer : public SocketServer { void Remove(Dispatcher* dispatcher); void Update(Dispatcher* dispatcher); -#if defined(WEBRTC_POSIX) - // Sets the function to be executed in response to the specified POSIX signal. - // The function is executed from inside Wait() using the "self-pipe trick"-- - // regardless of which thread receives the signal--and hence can safely - // manipulate user-level data structures. - // "handler" may be SIG_IGN, SIG_DFL, or a user-specified function, just like - // with signal(2). - // Only one PhysicalSocketServer should have user-level signal handlers. - // Dispatching signals on multiple PhysicalSocketServers is not reliable. - // The signal mask is not modified. It is the caller's responsibily to - // maintain it as desired. - virtual bool SetPosixSignalHandler(int signum, void (*handler)(int)); - - protected: - Dispatcher* signal_dispatcher(); -#endif - private: typedef std::set DispatcherSet; @@ -106,9 +86,6 @@ class RTC_EXPORT PhysicalSocketServer : public SocketServer { #if defined(WEBRTC_POSIX) bool WaitSelect(int cms, bool process_io); - static bool InstallSignal(int signum, void (*handler)(int)); - - std::unique_ptr signal_dispatcher_; #endif // WEBRTC_POSIX #if defined(WEBRTC_USE_EPOLL) void AddEpoll(Dispatcher* dispatcher); diff --git a/rtc_base/physical_socket_server_unittest.cc b/rtc_base/physical_socket_server_unittest.cc index 5083ca1791..586b9db292 100644 --- a/rtc_base/physical_socket_server_unittest.cc +++ b/rtc_base/physical_socket_server_unittest.cc @@ -501,139 +501,6 @@ TEST_F(PhysicalSocketTest, server_->set_network_binder(nullptr); } -class PosixSignalDeliveryTest : public ::testing::Test { - public: - static void RecordSignal(int signum) { - signals_received_.push_back(signum); - signaled_thread_ = Thread::Current(); - } - - protected: - void SetUp() override { ss_.reset(new PhysicalSocketServer()); } - - void TearDown() override { - ss_.reset(nullptr); - signals_received_.clear(); - signaled_thread_ = nullptr; - } - - bool ExpectSignal(int signum) { - if (signals_received_.empty()) { - RTC_LOG(LS_ERROR) << "ExpectSignal(): No signal received"; - return false; - } - if (signals_received_[0] != signum) { - RTC_LOG(LS_ERROR) << "ExpectSignal(): Received signal " - << signals_received_[0] << ", expected " << signum; - return false; - } - signals_received_.erase(signals_received_.begin()); - return true; - } - - bool ExpectNone() { - bool ret = signals_received_.empty(); - if (!ret) { - RTC_LOG(LS_ERROR) << "ExpectNone(): Received signal " - << signals_received_[0] << ", expected none"; - } - return ret; - } - - static std::vector signals_received_; - static Thread* signaled_thread_; - - std::unique_ptr ss_; -}; - -std::vector PosixSignalDeliveryTest::signals_received_; -Thread* PosixSignalDeliveryTest::signaled_thread_ = nullptr; - -// Test receiving a synchronous signal while not in Wait() and then entering -// Wait() afterwards. -// TODO(webrtc:7864): Fails on real iOS devices -#if defined(WEBRTC_IOS) && defined(WEBRTC_ARCH_ARM_FAMILY) -#define MAYBE_RaiseThenWait DISABLED_RaiseThenWait -#else -#define MAYBE_RaiseThenWait RaiseThenWait -#endif -TEST_F(PosixSignalDeliveryTest, MAYBE_RaiseThenWait) { - ASSERT_TRUE(ss_->SetPosixSignalHandler(SIGTERM, &RecordSignal)); - raise(SIGTERM); - EXPECT_TRUE(ss_->Wait(0, true)); - EXPECT_TRUE(ExpectSignal(SIGTERM)); - EXPECT_TRUE(ExpectNone()); -} - -// Test that we can handle getting tons of repeated signals and that we see all -// the different ones. -// TODO(webrtc:7864): Fails on real iOS devices -#if defined(WEBRTC_IOS) && defined(WEBRTC_ARCH_ARM_FAMILY) -#define MAYBE_InsanelyManySignals DISABLED_InsanelyManySignals -#else -#define MAYBE_InsanelyManySignals InsanelyManySignals -#endif -TEST_F(PosixSignalDeliveryTest, MAYBE_InsanelyManySignals) { - ss_->SetPosixSignalHandler(SIGTERM, &RecordSignal); - ss_->SetPosixSignalHandler(SIGINT, &RecordSignal); - for (int i = 0; i < 10000; ++i) { - raise(SIGTERM); - } - raise(SIGINT); - EXPECT_TRUE(ss_->Wait(0, true)); - // Order will be lowest signal numbers first. - EXPECT_TRUE(ExpectSignal(SIGINT)); - EXPECT_TRUE(ExpectSignal(SIGTERM)); - EXPECT_TRUE(ExpectNone()); -} - -// Test that a signal during a Wait() call is detected. -TEST_F(PosixSignalDeliveryTest, SignalDuringWait) { - ss_->SetPosixSignalHandler(SIGALRM, &RecordSignal); - alarm(1); - EXPECT_TRUE(ss_->Wait(1500, true)); - EXPECT_TRUE(ExpectSignal(SIGALRM)); - EXPECT_TRUE(ExpectNone()); -} - -// Test that it works no matter what thread the kernel chooses to give the -// signal to (since it's not guaranteed to be the one that Wait() runs on). -// TODO(webrtc:7864): Fails on real iOS devices -#if defined(WEBRTC_IOS) && defined(WEBRTC_ARCH_ARM_FAMILY) -#define MAYBE_SignalOnDifferentThread DISABLED_SignalOnDifferentThread -#else -#define MAYBE_SignalOnDifferentThread SignalOnDifferentThread -#endif -TEST_F(PosixSignalDeliveryTest, DISABLED_SignalOnDifferentThread) { - ss_->SetPosixSignalHandler(SIGTERM, &RecordSignal); - // Mask out SIGTERM so that it can't be delivered to this thread. - sigset_t mask; - sigemptyset(&mask); - sigaddset(&mask, SIGTERM); - EXPECT_EQ(0, pthread_sigmask(SIG_SETMASK, &mask, nullptr)); - // Start a new thread that raises it. It will have to be delivered to that - // thread. Our implementation should safely handle it and dispatch - // RecordSignal() on this thread. - std::unique_ptr thread(Thread::CreateWithSocketServer()); - thread->Start(); - thread->PostTask(RTC_FROM_HERE, [&thread]() { - thread->socketserver()->Wait(1000, false); - // Allow SIGTERM. This will be the only thread with it not masked so it will - // be delivered to us. - sigset_t mask; - sigemptyset(&mask); - pthread_sigmask(SIG_SETMASK, &mask, nullptr); - - // Raise it. - raise(SIGTERM); - }); - - EXPECT_TRUE(ss_->Wait(1500, true)); - EXPECT_TRUE(ExpectSignal(SIGTERM)); - EXPECT_EQ(Thread::Current(), signaled_thread_); - EXPECT_TRUE(ExpectNone()); -} - #endif } // namespace rtc