support multiple events listening on the same signal; make signals regular events that go on the same event queue

svn:r901

ChangeLog

@@ -117,6 +117,7 @@ Changes in current version:
  o Detect CLOCK_MONOTONIC at runtime for evdns; anonymous bug report	
  o Various HTTP correctness fixes from Scott Lamb
  o Fix a bug where deleting signals with the kqueue backend would cause subsequent adds to fail
+ o Support multiple events listening on the same signal; make signals regular events that go on the same event queue.
 	
 Changes in 1.4.0:
  o allow \r or \n individually to separate HTTP headers instead of the standard "\r\n"; from Charles Kerr.

event-internal.h

@@ -39,7 +39,6 @@ extern "C" {
 /* map union members back */
 
 /* mutually exclusive */
-#define ev_next		_ev.ev_io.ev_next
 #define ev_signal_next	_ev.ev_signal.ev_signal_next
 
 /* used only by signals */

event.c

@@ -244,7 +244,6 @@ event_base_new_with_config(struct event_config *cfg)
 
 	min_heap_ctor(&base->timeheap);
 	TAILQ_INIT(&base->eventqueue);
-	TAILQ_INIT(&base->sig.signalqueue);
 	base->sig.ev_signal_pair[0] = -1;
 	base->sig.ev_signal_pair[1] = -1;
 
@@ -678,7 +677,7 @@ event_base_loop(struct event_base *base, int flags)
 	struct timeval *tv_p;
 	int res, done;
 
-	if(!TAILQ_EMPTY(&base->sig.signalqueue))
+	if (&base->sig.ev_signal_added)
 		evsignal_base = base;
 	done = 0;
 	while (!done) {
@@ -939,13 +938,11 @@ event_pending(struct event *ev, short event, struct timeval *tv)
 	int flags = 0;
 
 	if (ev->ev_flags & EVLIST_INSERTED)
-		flags |= (ev->ev_events & (EV_READ|EV_WRITE));
+		flags |= (ev->ev_events & (EV_READ|EV_WRITE|EV_SIGNAL));
 	if (ev->ev_flags & EVLIST_ACTIVE)
 		flags |= ev->ev_res;
 	if (ev->ev_flags & EVLIST_TIMEOUT)
 		flags |= EV_TIMEOUT;
-	if (ev->ev_flags & EVLIST_SIGNAL)
-		flags |= EV_SIGNAL;
 
 	event &= (EV_TIMEOUT|EV_READ|EV_WRITE|EV_SIGNAL);
 
@@ -1026,7 +1023,7 @@ event_add_internal(struct event *ev, const struct timeval *tv)
 		 * removes it from the active list. */
 		if ((ev->ev_flags & EVLIST_ACTIVE) &&
 		    (ev->ev_res & EV_TIMEOUT)) {
-			if (ev->ev_flags & EVLIST_SIGNAL) {
+			if (ev->ev_events & EV_SIGNAL) {
 				/* See if we are just active executing
 				 * this event in a loop
 				 */
@@ -1049,16 +1046,11 @@ event_add_internal(struct event *ev, const struct timeval *tv)
 		event_queue_insert(base, ev, EVLIST_TIMEOUT);
 	}
 
-	if ((ev->ev_events & (EV_READ|EV_WRITE)) &&
+	if ((ev->ev_events & (EV_READ|EV_WRITE|EV_SIGNAL)) &&
 	    !(ev->ev_flags & (EVLIST_INSERTED|EVLIST_ACTIVE))) {
 		res = evsel->add(evbase, ev);
 		if (res != -1)
 			event_queue_insert(base, ev, EVLIST_INSERTED);
-	} else if ((ev->ev_events & EV_SIGNAL) &&
-	    !(ev->ev_flags & EVLIST_SIGNAL)) {
-		res = evsel->add(evbase, ev);
-		if (res != -1)
-			event_queue_insert(base, ev, EVLIST_SIGNAL);
 	}
 
 	/* if we are not in the right thread, we need to wake up the loop */
@@ -1104,7 +1096,7 @@ event_del_internal(struct event *ev)
 	assert(!(ev->ev_flags & ~EVLIST_ALL));
 
 	/* See if we are just active executing this event in a loop */
-	if (ev->ev_flags & EVLIST_SIGNAL) {
+	if (ev->ev_events & EV_SIGNAL) {
 		if (ev->ev_ncalls && ev->ev_pncalls) {
 			/* Abort loop */
 			*ev->ev_pncalls = 0;
@@ -1120,9 +1112,6 @@ event_del_internal(struct event *ev)
 	if (ev->ev_flags & EVLIST_INSERTED) {
 		event_queue_remove(base, ev, EVLIST_INSERTED);
 		res = evsel->del(evbase, ev);
-	} else if (ev->ev_flags & EVLIST_SIGNAL) {
-		event_queue_remove(base, ev, EVLIST_SIGNAL);
-		res = evsel->del(evbase, ev);
 	}
 
 	/* if we are not in the right thread, we need to wake up the loop */
@@ -1158,7 +1147,7 @@ event_active_internal(struct event *ev, int res, short ncalls)
 
 	ev->ev_res = res;
 
-	if (ev->ev_flags & EVLIST_SIGNAL) {
+	if (ev->ev_events & EV_SIGNAL) {
 		ev->ev_ncalls = ncalls;
 		ev->ev_pncalls = NULL;
 	}
@@ -1298,9 +1287,6 @@ event_queue_remove(struct event_base *base, struct event *ev, int queue)
 	case EVLIST_TIMEOUT:
 		min_heap_erase(&base->timeheap, ev);
 		break;
-	case EVLIST_SIGNAL:
-		TAILQ_REMOVE(&base->sig.signalqueue, ev, ev_signal_next);
-		break;
 	default:
 		event_errx(1, "%s: unknown queue %x", __func__, queue);
 	}
@@ -1335,9 +1321,6 @@ event_queue_insert(struct event_base *base, struct event *ev, int queue)
 		min_heap_push(&base->timeheap, ev);
 		break;
 	}
-	case EVLIST_SIGNAL:
-		TAILQ_INSERT_TAIL(&base->sig.signalqueue, ev, ev_signal_next);
-		break;
 	default:
 		event_errx(1, "%s: unknown queue %x", __func__, queue);
 	}

evsignal.h

@@ -34,11 +34,11 @@
 typedef void (*ev_sighandler_t)(int);
 
 struct evsignal_info {
-	struct event_list signalqueue;
 	struct event ev_signal;
-    evutil_socket_t ev_signal_pair[2];
+	evutil_socket_t ev_signal_pair[2];
 	int ev_signal_added;
 	volatile sig_atomic_t evsignal_caught;
+	struct event_list evsigevents[NSIG];
 	sig_atomic_t evsigcaught[NSIG];
 #ifdef HAVE_SIGACTION
 	struct sigaction **sh_old;

http.c

@@ -1575,7 +1575,8 @@ evhttp_read_firstline(struct evhttp_connection *evcon,
 	res = evhttp_parse_firstline(req, bufferevent_get_input(evcon->bufev));
 	if (res == DATA_CORRUPTED) {
 		/* Error while reading, terminate */
-		event_debug(("%s: bad header lines on %d\n", __func__, fd));
+		event_debug(("%s: bad header lines on %d\n",
+			__func__, evcon->fd));
 		evhttp_connection_fail(evcon, EVCON_HTTP_INVALID_HEADER);
 		return;
 	} else if (res == MORE_DATA_EXPECTED) {

include/event2/event_struct.h

@@ -78,17 +78,13 @@ struct {								\
 struct event_base;
 struct event {
 	TAILQ_ENTRY (event) (ev_active_next);
+	TAILQ_ENTRY (event) (ev_next);
 	int min_heap_idx;	/* for managing timeouts */
 
 	struct event_base *ev_base;
 
 	evutil_socket_t ev_fd;
 	union {
-		/* used by io events */
-		struct {
-			TAILQ_ENTRY (event) (ev_next);
-		} ev_io;
-
 		/* used by signal events */
 		struct {
 			TAILQ_ENTRY (event) (ev_signal_next);

kqueue.c

@@ -44,6 +44,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <errno.h>
+#include <assert.h>
 #ifdef HAVE_INTTYPES_H
 #include <inttypes.h>
 #endif
@@ -71,6 +72,7 @@ struct kqop {
 	struct kevent *changes;
 	int nchanges;
 	struct kevent *events;
+	struct event_list evsigevents[NSIG];
 	int nevents;
 	int kq;
 	pid_t pid;
@@ -97,7 +99,7 @@ const struct eventop kqops = {
 static void *
 kq_init(struct event_base *base)
 {
-	int kq;
+	int i, kq;
 	struct kqop *kqueueop;
 
 	if (!(kqueueop = mm_calloc(1, sizeof(struct kqop))))
@@ -129,6 +131,11 @@ kq_init(struct event_base *base)
 	}
 	kqueueop->nevents = NEVENT;
 
+	/* we need to keep track of multiple events per signal */
+	for (i = 0; i < NSIG; ++i) {
+		TAILQ_INIT(&kqueueop->evsigevents[i]);
+	}
+
 	/* Check for Mac OS X kqueue bug. */
 	kqueueop->changes[0].ident = -1;
 	kqueueop->changes[0].filter = EVFILT_READ;
@@ -257,8 +264,6 @@ kq_dispatch(struct event_base *base, void *arg, struct timeval *tv)
 			return (-1);
 		}
 
-		ev = (struct event *)events[i].udata;
-
 		if (events[i].filter == EVFILT_READ) {
 			which |= EV_READ;
 		} else if (events[i].filter == EVFILT_WRITE) {
@@ -270,11 +275,20 @@ kq_dispatch(struct event_base *base, void *arg, struct timeval *tv)
 		if (!which)
 			continue;
 
-		if (!(ev->ev_events & EV_PERSIST))
-			ev->ev_flags &= ~EVLIST_X_KQINKERNEL;
+		if (events[i].filter == EVFILT_SIGNAL) {
+			struct event_list *head =
+			    (struct event_list *)events[i].udata;
+			TAILQ_FOREACH(ev, head, ev_signal_next) {
+				event_active(ev, which, events[i].data);
+			}
+		} else {
+			ev = (struct event *)events[i].udata;
 
-		event_active(ev, which | (ev->ev_events & EV_ET),
-		    ev->ev_events & EV_SIGNAL ? events[i].data : 1);
+			if (!(ev->ev_events & EV_PERSIST))
+				ev->ev_flags &= ~EVLIST_X_KQINKERNEL;
+
+			event_active(ev, which | (ev->ev_events & EV_ET), 1);
+		}
 	}
 
 	return (0);
@@ -289,25 +303,30 @@ kq_add(void *arg, struct event *ev)
 
 	if (ev->ev_events & EV_SIGNAL) {
 		int nsignal = EVENT_SIGNAL(ev);
-                struct timespec timeout = { 0, 0 };
 
- 		memset(&kev, 0, sizeof(kev));
-		kev.ident = nsignal;
-		kev.filter = EVFILT_SIGNAL;
-		kev.flags = EV_ADD;
-		if (!(ev->ev_events & EV_PERSIST))
-			kev.flags |= EV_ONESHOT;
-		kev.udata = PTR_TO_UDATA(ev);
-
-		/* Be ready for the signal if it is sent any time between
-		 * now and the next call to kq_dispatch. */
-                if (kevent(kqop->kq, &kev, 1, NULL, 0, &timeout) == -1)
-                	return (-1);
-
-		if (_evsignal_set_handler(ev->ev_base, nsignal,
-					  kq_sighandler) == -1)
-			return (-1);
+		assert(nsignal >= 0 && nsignal < NSIG);
+		if (TAILQ_EMPTY(&kqop->evsigevents[nsignal])) {
+			struct timespec timeout = { 0, 0 };
+			
+			memset(&kev, 0, sizeof(kev));
+			kev.ident = nsignal;
+			kev.filter = EVFILT_SIGNAL;
+			kev.flags = EV_ADD;
+			kev.udata = PTR_TO_UDATA(&kqop->evsigevents[nsignal]);
+			
+			/* Be ready for the signal if it is sent any
+			 * time between now and the next call to
+			 * kq_dispatch. */
+			if (kevent(kqop->kq, &kev, 1, NULL, 0, &timeout) == -1)
+				return (-1);
+			
+			if (_evsignal_set_handler(ev->ev_base, nsignal,
+				kq_sighandler) == -1)
+				return (-1);
+		}
 
+		TAILQ_INSERT_TAIL(&kqop->evsigevents[nsignal], ev,
+		    ev_signal_next);
 		ev->ev_flags |= EVLIST_X_KQINKERNEL;
 		return (0);
 	}
@@ -366,18 +385,24 @@ kq_del(void *arg, struct event *ev)
 		int nsignal = EVENT_SIGNAL(ev);
 		struct timespec timeout = { 0, 0 };
 
- 		memset(&kev, 0, sizeof(kev));
-		kev.ident = nsignal;
-		kev.filter = EVFILT_SIGNAL;
-		kev.flags = EV_DELETE;
+		assert(nsignal >= 0 && nsignal < NSIG);
+		TAILQ_REMOVE(&kqop->evsigevents[nsignal], ev, ev_signal_next);
+		if (TAILQ_EMPTY(&kqop->evsigevents[nsignal])) {
+			memset(&kev, 0, sizeof(kev));
+			kev.ident = nsignal;
+			kev.filter = EVFILT_SIGNAL;
+			kev.flags = EV_DELETE;
 		
-		/* Because we insert signal events immediately, we need to
-		 * delete them immediately, too */
-                if (kevent(kqop->kq, &kev, 1, NULL, 0, &timeout) == -1)
-                	return (-1);
+			/* Because we insert signal events
+			 * immediately, we need to delete them
+			 * immediately, too */
+			if (kevent(kqop->kq, &kev, 1, NULL, 0, &timeout) == -1)
+				return (-1);
 
-		if (_evsignal_restore_handler(ev->ev_base, nsignal) == -1)
-			return (-1);
+			if (_evsignal_restore_handler(ev->ev_base,
+				nsignal) == -1)
+				return (-1);
+		}
 
 		ev->ev_flags &= ~EVLIST_X_KQINKERNEL;
 		return (0);

signal.c

@@ -97,12 +97,15 @@ evsignal_cb(evutil_socket_t fd, short what, void *arg)
 void
 evsignal_init(struct event_base *base)
 {
+	int i;
+
 	/* 
 	 * Our signal handler is going to write to one end of the socket
 	 * pair to wake up our event loop.  The event loop then scans for
 	 * signals that got delivered.
 	 */
-	if (evutil_socketpair(AF_UNIX, SOCK_STREAM, 0, base->sig.ev_signal_pair) == -1)
+	if (evutil_socketpair(
+		    AF_UNIX, SOCK_STREAM, 0, base->sig.ev_signal_pair) == -1)
 		event_err(1, "%s: socketpair", __func__);
 
 	FD_CLOSEONEXEC(base->sig.ev_signal_pair[0]);
@@ -111,6 +114,9 @@ evsignal_init(struct event_base *base)
 	base->sig.sh_old_max = 0;
 	base->sig.evsignal_caught = 0;
 	memset(&base->sig.evsigcaught, 0, sizeof(sig_atomic_t)*NSIG);
+	/* initialize the queues for all events */
+	for (i = 0; i < NSIG; ++i)
+		TAILQ_INIT(&base->sig.evsigevents[i]);
 
         evutil_make_socket_nonblocking(base->sig.ev_signal_pair[0]);
 
@@ -189,20 +195,26 @@ evsignal_add(struct event *ev)
 	struct evsignal_info *sig = &ev->ev_base->sig;
 
 	evsignal = EVENT_SIGNAL(ev);
-
-	event_debug(("%s: %p: changing signal handler", __func__, ev));
-	if (_evsignal_set_handler(base, evsignal, evsignal_handler) == -1)
-		return (-1);
-
-	/* catch signals if they happen quickly */
-	evsignal_base = base;
-
-	if (!sig->ev_signal_added) {
-		if (event_add(&sig->ev_signal, NULL))
+	assert(evsignal >= 0 & evsignal < NSIG);
+	if (TAILQ_EMPTY(&sig->evsigevents[evsignal])) {
+		event_debug(("%s: %p: changing signal handler", __func__, ev));
+		if (_evsignal_set_handler(
+			    base, evsignal, evsignal_handler) == -1)
 			return (-1);
-		sig->ev_signal_added = 1;
+
+		/* catch signals if they happen quickly */
+		evsignal_base = base;
+
+		if (!sig->ev_signal_added) {
+			if (event_add(&sig->ev_signal, NULL))
+				return (-1);
+			sig->ev_signal_added = 1;
+		}
 	}
 
+	/* multiple events may listen to the same signal */
+	TAILQ_INSERT_TAIL(&sig->evsigevents[evsignal], ev, ev_signal_next);
+
 	return (0);
 }
 
@@ -240,8 +252,21 @@ _evsignal_restore_handler(struct event_base *base, int evsignal)
 int
 evsignal_del(struct event *ev)
 {
+	struct event_base *base = ev->ev_base;
+	struct evsignal_info *sig = &base->sig;
+	int evsignal = EVENT_SIGNAL(ev);
+
+	assert(evsignal >= 0 & evsignal < NSIG);
+
+	/* multiple events may listen to the same signal */
+	TAILQ_REMOVE(&sig->evsigevents[evsignal], ev, ev_signal_next);
+
+	if (!TAILQ_EMPTY(&sig->evsigevents[evsignal]))
+		return (0);
+
 	event_debug(("%s: %p: restoring signal handler", __func__, ev));
-	return _evsignal_restore_handler(ev->ev_base, EVENT_SIGNAL(ev));
+
+	return (_evsignal_restore_handler(ev->ev_base, EVENT_SIGNAL(ev)));
 }
 
 static void
@@ -249,7 +274,7 @@ evsignal_handler(int sig)
 {
 	int save_errno = errno;
 
-	if(evsignal_base == NULL) {
+	if (evsignal_base == NULL) {
 		event_warn(
 			"%s: received signal %d, but have no base configured",
 			__func__, sig);
@@ -271,29 +296,39 @@ evsignal_handler(int sig)
 void
 evsignal_process(struct event_base *base)
 {
-	struct event *ev;
+	struct evsignal_info *sig = &base->sig;
+	struct event *ev, *next_ev;
 	sig_atomic_t ncalls;
-
+	int i;
+	
 	base->sig.evsignal_caught = 0;
-	TAILQ_FOREACH(ev, &base->sig.signalqueue, ev_signal_next) {
-		ncalls = base->sig.evsigcaught[EVENT_SIGNAL(ev)];
-		if (ncalls) {
+	for (i = 1; i < NSIG; ++i) {
+		ncalls = sig->evsigcaught[i];
+		if (ncalls == 0)
+			continue;
+
+		for (ev = TAILQ_FIRST(&sig->evsigevents[i]);
+		    ev != NULL; ev = next_ev) {
+			next_ev = TAILQ_NEXT(ev, ev_signal_next);
 			if (!(ev->ev_events & EV_PERSIST))
 				event_del(ev);
 			event_active(ev, EV_SIGNAL, ncalls);
-			base->sig.evsigcaught[EVENT_SIGNAL(ev)] = 0;
 		}
+
+		sig->evsigcaught[i] = 0;
 	}
 }
 
 void
 evsignal_dealloc(struct event_base *base)
 {
-	if(base->sig.ev_signal_added) {
+	int i = 0;
+	if (base->sig.ev_signal_added) {
 		event_del(&base->sig.ev_signal);
 		base->sig.ev_signal_added = 0;
 	}
-	assert(TAILQ_EMPTY(&base->sig.signalqueue));
+	for (i = 0; i < NSIG; ++i)
+		assert(TAILQ_EMPTY(&base->sig.evsigevents[0]));
 
 	EVUTIL_CLOSESOCKET(base->sig.ev_signal_pair[0]);
 	base->sig.ev_signal_pair[0] = -1;

test/regress.c

@@ -577,6 +577,36 @@ test_simplesignal(void)
 	cleanup_test();
 }
 
+static void
+test_multiplesignal(void)
+{
+	struct event ev_one, ev_two;
+	struct itimerval itv;
+
+	setup_test("Multiple signal: ");
+
+	signal_set(&ev_one, SIGALRM, signal_cb, &ev_one);
+	signal_add(&ev_one, NULL);
+
+	signal_set(&ev_two, SIGALRM, signal_cb, &ev_two);
+	signal_add(&ev_two, NULL);
+
+	memset(&itv, 0, sizeof(itv));
+	itv.it_value.tv_sec = 1;
+	if (setitimer(ITIMER_REAL, &itv, NULL) == -1)
+		goto skip_simplesignal;
+
+	event_dispatch();
+
+ skip_simplesignal:
+	if (signal_del(&ev_one) == -1)
+		test_ok = 0;
+	if (signal_del(&ev_two) == -1)
+		test_ok = 0;
+
+	cleanup_test();
+}
+
 static void
 test_immediatesignal(void)
 {
@@ -2261,8 +2291,9 @@ main (int argc, char **argv)
 	test_simpletimeout();
 
 #ifndef WIN32
-    test_edgetriggered();
+	test_edgetriggered();
 	test_simplesignal();
+	test_multiplesignal();
 	test_immediatesignal();
 #endif
 	test_loopexit();