Skip to content

Commit

Permalink
Cleanup in signal.c and related tests
Browse files Browse the repository at this point in the history 
Add comments, add assert for edge case around signal registration, and
make consistent *always* using atomic get/set for memory shared
between the signal handler and the threads.
  • Loading branch information
@xmcclure
xmcclure committed Dec 2, 2015
1 parent d94ba5b commit bca3964
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 38 deletions.
2 changes: 2 additions & 0 deletions mcs/class/Mono.Posix/Mono.Unix/UnixSignal.cs
View file
Expand Up @@ -145,6 +145,8 @@ public unsafe bool Reset ()
set {Interlocked.Exchange (ref Info->count, value);}
}

// signum, count, write_fd, pipecnt, and pipelock are read from a signal handler thread
// count and pipelock are both read and written from the signal handler thread
[Map]
struct SignalInfo {
public int signum, count, read_fd, write_fd, pipecnt, pipelock, have_handler;
Expand Down
3 changes: 3 additions & 0 deletions mcs/class/Mono.Posix/Test/Mono.Unix/UnixSignalTest.cs
View file
Expand Up @@ -456,6 +456,7 @@ static void CloseSignals (UnixSignal[] signals)
s.Close ();
}

// Create thread that issues many signals from a set of harmless signals
static Thread CreateRaiseStormThread (int max)
{
return new Thread (delegate () {
Expand Down Expand Up @@ -486,6 +487,7 @@ public void TestAddRemove ()
CloseSignals (usignals);
}

// Create thread that repeatedly registers then unregisters signal handlers
static Thread CreateSignalCreatorThread ()
{
return new Thread (delegate () {
Expand Down Expand Up @@ -523,6 +525,7 @@ public void TestWaitAny ()
CloseSignals (usignals);
}

// Create thread that blocks until at least one of the given signals is received
static Thread CreateWaitAnyThread (params UnixSignal[] usignals)
{
return new Thread (delegate () {
Expand Down
98 changes: 60 additions & 38 deletions support/signal.c
View file
Expand Up @@ -107,6 +107,10 @@ int Mono_Posix_FromRealTimeSignum (int offset, int *r)

#ifndef HOST_WIN32

// Atomicity rules: Fields of signal_info read or written by the signal handler
// (see UnixSignal.cs) should be read and written using atomic functions.
// (For simplicity, we're protecting some things we don't strictly need to.)

// Because we are in MonoPosixHelper, we are banned from linking mono.
// We can still use atomic.h because that's all inline functions--
// unless WAPI_NO_ATOMIC_ASM is defined, in which case atomic.h calls linked functions.
Expand Down Expand Up @@ -170,9 +174,9 @@ keep_trying (int r)
// This tiny ad-hoc read/write lock is needed because of the very specific
// synchronization needed between default_handler and teardown_pipes:
// - Many default_handlers can be running at once
// - External forces already ensure only one teardown_pipes runs at once
// - If a teardown_pipes interrupts a default_handler, it must block
// - If a default_handler interrupts a teardown_pipes, it must *not* block
// - The signals_mutex already ensures only one teardown_pipes runs at once
// - If teardown_pipes starts while a default_handler is ongoing, it must block
// - If default_handler starts while a teardown_pipes is ongoing, it must *not* block
// Locks are implemented as ints.

// The lock is split into a teardown bit and a handler count (sign bit unused).
Expand All @@ -196,7 +200,7 @@ acquire_pipelock_teardown (int *lock)
}
// Now wait for all handlers to complete.
while (1) {
if (0 == PIPELOCK_GET_COUNT(lockvalue_draining))
if (0 == PIPELOCK_GET_COUNT (lockvalue_draining))
break; // We now hold the lock.
// Handler is still running, spin until it completes.
sched_yield (); // We can call this because !defined(HOST_WIN32)
Expand Down Expand Up @@ -267,10 +271,10 @@ default_handler (int signum)
continue; // Teardown is occurring on this object, no one to send to.

fd = mph_int_get (&h->write_fd);
if (fd > 0) {
if (fd > 0) { // If any listener exists to write to
int j,pipecounter;
char c = signum;
pipecounter = mph_int_get (&h->pipecnt);
char c = signum; // (Value is meaningless)
pipecounter = mph_int_get (&h->pipecnt); // Write one byte per pipe listener
for (j = 0; j < pipecounter; ++j) {
int r;
do { r = write (fd, &c, 1); } while (keep_trying (r));
Expand All @@ -282,13 +286,14 @@ default_handler (int signum)

static pthread_mutex_t signals_mutex = PTHREAD_MUTEX_INITIALIZER;

// A UnixSignal object is being constructed
void*
Mono_Unix_UnixSignal_install (int sig)
{
#if defined(HAVE_SIGNAL)
int i;
signal_info* h = NULL;
int have_handler = 0;
signal_info* h = NULL; // signals[] slot to install to
int have_handler = 0; // Candidates for signal_info handler fields
void* handler = NULL;

if (acquire_mutex (&signals_mutex) == -1)
Expand All @@ -302,13 +307,16 @@ Mono_Unix_UnixSignal_install (int sig)
if (sinfo.sa_handler != SIG_DFL || (void*)sinfo.sa_sigaction != (void*)SIG_DFL) {
pthread_mutex_unlock (&signals_mutex);
errno = EADDRINUSE;
return NULL;
return NULL; // This is an rt signal with an existing handler. Bail out.
}
}
#endif /*defined (SIGRTMIN) && defined (SIGRTMAX)*/

// Scan through signals list looking for (1) an unused spot (2) a usable value for handler
for (i = 0; i < NUM_SIGNALS; ++i) {
if (h == NULL && signals [i].signum == 0) {
int just_installed = 0;
// We're still looking for a signal_info spot, and this one is available:
if (h == NULL && mph_int_get (&signals [i].signum) == 0) {
h = &signals [i];
h->handler = signal (sig, default_handler);
if (h->handler == SIG_ERR) {
Expand All @@ -317,27 +325,32 @@ Mono_Unix_UnixSignal_install (int sig)
break;
}
else {
h->have_handler = 1;
just_installed = 1;
}
}
if (!have_handler && signals [i].signum == sig &&
// Check if this slot has a "usable" (not installed by this file) handler-to-restore-later:
// (On the first signal to be installed, signals [i] will be == h when this happens.)
if (!have_handler && (just_installed || mph_int_get (&signals [i].signum) == sig) &&
signals [i].handler != default_handler) {
have_handler = 1;
handler = signals [i].handler;
}
if (h && have_handler)
if (h && have_handler) // We have everything we need
break;
}

if (h && have_handler) {
if (h) {
// If we reached here without have_handler, this means that default_handler
// was set as the signal handler before the first UnixSignal object was installed.
g_assert (have_handler);

// Overwrite the tenative handler we set a moment ago with a known-usable one
h->handler = handler;
h->have_handler = 1;
h->handler = handler;
}

if (h) {
mph_int_set (&h->count, 0);
mph_int_set (&h->signum, sig);
mph_int_set (&h->pipecnt, 0);
mph_int_set (&h->signum, sig);
}

release_mutex (&signals_mutex);
Expand All @@ -355,16 +368,17 @@ count_handlers (int signum)
int i;
int count = 0;
for (i = 0; i < NUM_SIGNALS; ++i) {
if (signals [i].signum == signum)
if (mph_int_get (&signals [i].signum) == signum)
++count;
}
return count;
}

// A UnixSignal object is being Disposed
int
Mono_Unix_UnixSignal_uninstall (void* info)
{
#if defined(HAVE_SIGNAL)
#if defined(HAVE_SIGNAL)
signal_info* h;
int r = -1;

Expand All @@ -377,14 +391,15 @@ Mono_Unix_UnixSignal_uninstall (void* info)
errno = EINVAL;
else {
/* last UnixSignal -- we can unregister */
if (h->have_handler && count_handlers (h->signum) == 1) {
mph_sighandler_t p = signal (h->signum, h->handler);
int signum = mph_int_get (&h->signum);
if (h->have_handler && count_handlers (signum) == 1) {
mph_sighandler_t p = signal (signum, h->handler);
if (p != SIG_ERR)
r = 0;
h->handler = NULL;
h->have_handler = 0;
}
h->signum = 0;
mph_int_set (&h->signum, 0);
}

release_mutex (&signals_mutex);
Expand All @@ -396,6 +411,7 @@ Mono_Unix_UnixSignal_uninstall (void* info)
#endif
}

// Set up a signal_info to begin waiting for signal
static int
setup_pipes (signal_info** signals, int count, struct pollfd *fd_structs, int *currfd)
{
Expand All @@ -407,45 +423,51 @@ setup_pipes (signal_info** signals, int count, struct pollfd *fd_structs, int *c

h = signals [i];

if (mph_int_get (&h->pipecnt) == 0) {
if (mph_int_get (&h->pipecnt) == 0) { // First listener for this signal_info
if ((r = pipe (filedes)) != 0) {
break;
}
h->read_fd = filedes [0];
h->write_fd = filedes [1];
mph_int_set (&h->read_fd, filedes [0]);
mph_int_set (&h->write_fd, filedes [1]);
}
mph_int_inc (&h->pipecnt);
fd_structs[*currfd].fd = h->read_fd;
fd_structs[*currfd].fd = mph_int_get (&h->read_fd);
fd_structs[*currfd].events = POLLIN;
++(*currfd);
++(*currfd); // count is verified less than NUM_SIGNALS by caller
}
return r;
}

// Cleanup a signal_info after waiting for signal
static void
teardown_pipes (signal_info** signals, int count)
{
int i;
for (i = 0; i < count; ++i) {
signal_info* h = signals [i];

if (mph_int_dec_test (&h->pipecnt)) {
if (mph_int_dec_test (&h->pipecnt)) { // Final listener for this signal_info
acquire_pipelock_teardown (&h->pipelock);
if (h->read_fd != 0)
close (h->read_fd);
if (h->write_fd != 0)
close (h->write_fd);
h->read_fd = 0;
h->write_fd = 0;
int read_fd = mph_int_get (&h->read_fd);
int write_fd = mph_int_get (&h->write_fd);
if (read_fd != 0)
close (read_fd);
if (write_fd != 0)
close (write_fd);
mph_int_set (&h->read_fd, 0);
mph_int_set (&h->write_fd, 0);
release_pipelock_teardown (&h->pipelock);
}
}
}

// Given pipes set up, wait for a byte to arrive on one of them
static int
wait_for_any (signal_info** signals, int count, int *currfd, struct pollfd* fd_structs, int timeout, Mono_Posix_RuntimeIsShuttingDown shutting_down)
{
int r, idx;
// Poll until one of this signal_info's pipes is ready to read.
// Once a second, stop to check if the VM is shutting down.
do {
struct timeval tv;
struct timeval *ptv = NULL;
Expand All @@ -460,15 +482,15 @@ wait_for_any (signal_info** signals, int count, int *currfd, struct pollfd* fd_s
idx = -1;
if (r == 0)
idx = timeout;
else if (r > 0) {
else if (r > 0) { // The pipe[s] are ready to read.
int i;
for (i = 0; i < count; ++i) {
signal_info* h = signals [i];
if (fd_structs[i].revents & POLLIN) {
int r;
char c;
do {
r = read (h->read_fd, &c, 1);
r = read (mph_int_get (&h->read_fd), &c, 1);
} while (keep_trying (r) && !shutting_down ());
if (idx == -1)
idx = i;
Expand Down

0 comments on commit bca3964

Please sign in to comment.