Reland "Delete unused code to handle posix signals in PhysicalSocketServer"
This is a reland of d2490aef20457f4e981e5cc14e84552389d2363b
Earlier link errors were likely a single trybot with corrupted dep files.
Original change's description:
> Delete unused code to handle posix signals in PhysicalSocketServer
>
> Bug: None
> Change-Id: I3abddef4f1af5499f39a8d3f643c779effe9e01d
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175006
> Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
> Commit-Queue: Niels Moller <nisse@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#31237}
Bug: webrtc:11571
Change-Id: I7ea14f26a2186a9d51a75493b7280fc0ad6b8c77
Tbr: kwiberg@webrtc.org
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175042
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31251}
diff --git a/rtc_base/physical_socket_server.cc b/rtc_base/physical_socket_server.cc
index f52701c..cf65300 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 <poll.h>
#endif
-#include <signal.h>
#include <sys/ioctl.h>
#include <sys/select.h>
#include <sys/time.h>
@@ -959,181 +958,6 @@
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<int>(arraysize(received_signal_)));
- if (signum < static_cast<int>(arraysize(received_signal_))) {
- return received_signal_[signum];
- } else {
- return false;
- }
- }
-
- // Clears the given signal number.
- void ClearSignal(int signum) {
- RTC_DCHECK(signum < static_cast<int>(arraysize(received_signal_)));
- if (signum < static_cast<int>(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<int>(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<void*>(static_cast<volatile void*>(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<int, void (*)(int)> HandlerMap;
-
- HandlerMap handlers_;
- // Our owner.
- PhysicalSocketServer* owner_;
-};
-
#endif // WEBRTC_POSIX
#if defined(WEBRTC_WIN)
@@ -1231,9 +1055,6 @@
#if defined(WEBRTC_WIN)
WSACloseEvent(socket_ev_);
#endif
-#if defined(WEBRTC_POSIX)
- signal_dispatcher_.reset();
-#endif
delete signal_wakeup_;
#if defined(WEBRTC_USE_EPOLL)
if (epoll_fd_ != INVALID_SOCKET) {
@@ -1750,62 +1571,6 @@
#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 01e91f3..e7985db 100644
--- a/rtc_base/physical_socket_server.h
+++ b/rtc_base/physical_socket_server.h
@@ -41,9 +41,6 @@
};
class Signaler;
-#if defined(WEBRTC_POSIX)
-class PosixSignalDispatcher;
-#endif
class Dispatcher {
public:
@@ -82,23 +79,6 @@
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<Dispatcher*> DispatcherSet;
@@ -106,9 +86,6 @@
#if defined(WEBRTC_POSIX)
bool WaitSelect(int cms, bool process_io);
- static bool InstallSignal(int signum, void (*handler)(int));
-
- std::unique_ptr<PosixSignalDispatcher> 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 5083ca1..586b9db 100644
--- a/rtc_base/physical_socket_server_unittest.cc
+++ b/rtc_base/physical_socket_server_unittest.cc
@@ -501,139 +501,6 @@
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<int> signals_received_;
- static Thread* signaled_thread_;
-
- std::unique_ptr<PhysicalSocketServer> ss_;
-};
-
-std::vector<int> 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(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
