Change ident for EVFILT_USER to 0 and add a test (#1582)

Conventionally, ident for EVFILT_USER is set to 0 to avoid
collision of file descriptors, which is what other renowned
networking frameworks like netty(java), mio(rust), gnet(go),
swift-nio(swift), etc. do currently.

Co-authored-by: Azat Khuzhin <azat@libevent.org>

.gitignore

@@ -126,6 +126,7 @@ test-time
 test-weof
 test-changelist
 test-fdleak
+test-kq-collision
 
 event-config.h
 evconfig-private.h

CMakeLists.txt

@@ -1263,6 +1263,7 @@ macro(add_test_prog prog)
                           event_extra
                           ${ARGN})
 endmacro()
+
 if (NOT EVENT__DISABLE_TESTS)
     #
     # Generate Regress tests.
@@ -1403,6 +1404,12 @@ if (NOT EVENT__DISABLE_TESTS)
         test-ratelim
     )
 
+    if(PTHREADS_AVAILABLE AND EVENT__HAVE_KQUEUE)
+        add_test_prog(test-kq-collision event_pthreads)
+        list(APPEND ALL_TESTPROGS test-kq-collision)
+        add_test(test-test-kq-collision ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/test-kq-collision)
+    endif()
+
     #
     # We run all tests with the different backends turned on one at a time.
     #

kqueue.c

@@ -512,11 +512,16 @@ kq_sig_del(struct event_base *base, int nsignal, short old, short events, void *
 }
 
 
-/* OSX 10.6 and FreeBSD 8.1 add support for EVFILT_USER, which we can use
- * to wake up the event loop from another thread. */
+/* OSX 10.6, FreeBSD 8.1, DragonFlyBSD 4.0 and NetBSD 10.0 added support for EVFILT_USER,
+ * which we can use to wake up the event loop from another thread. */
 
-/* Magic number we use for our filter ID. */
-#define NOTIFY_IDENT 42
+/* Magic number we use for our filter ID.
+ *
+ * This is a made-up value, so it can be any integer within the range of type uintptr_t,
+ * it's used in conjunction with filter as a (ident, filter) pair to identify a event entry.
+ * We use 0 for consistency with other mainstream networking libraries.
+ */
+#define NOTIFY_IDENT 0
 
 int
 event_kq_add_notify_event_(struct event_base *base)

test/include.am

@@ -28,8 +28,15 @@ TESTPROGRAMS = \
 	test/test-init				\
 	test/test-ratelim				\
 	test/test-time				\
-	test/test-weof \
-	test/regress
+	test/test-weof
+
+if PTHREADS
+if KQUEUE_BACKEND
+TESTPROGRAMS += test/test-kq-collision
+endif
+endif
+
+TESTPROGRAMS += test/regress
 
 if BUILD_REGRESS
 noinst_PROGRAMS += $(TESTPROGRAMS)
@@ -108,6 +115,15 @@ test_test_ratelim_LDADD = libevent_core.la -lm
 test_test_fdleak_SOURCES = test/test-fdleak.c
 test_test_fdleak_LDADD = libevent_core.la
 
+if PTHREADS
+if KQUEUE_BACKEND
+test_test_kq_collision_SOURCES = test/test-kq-collision.c
+test_test_kq_collision_LDADD = libevent_core.la libevent_pthreads.la
+test_test_kq_collision_CPPFLAGS = $(AM_CPPFLAGS) $(PTHREAD_CFLAGS) -Itest
+test_test_kq_collision_LDFLAGS = $(PTHREAD_CFLAGS)
+endif
+endif
+
 test_regress_SOURCES = 				\
 	test/regress.c				\
 	test/regress.gen.c				\

test/test-kq-collision.c

@@ -0,0 +1,204 @@
+/*
+ * Copyright (c) 2024 Andy Pan <i@andypan.me>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. The name of the author may not be used to endorse or promote products
+ *    derived from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+ * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <assert.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#ifdef EVENT__HAVE_SYS_TIME_H
+#include <sys/time.h>
+#endif
+#include <fcntl.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <pthread.h>
+
+#include <event2/event.h>
+#include <event2/util.h>
+#include <event2/thread.h>
+#include "event-internal.h"
+
+struct timeval timeout = {3, 0};
+char data[] = "Hello, World!";
+int read_called = 0;
+
+#define MAGIC_FD 42 // The old magic number used by kqueue EVFILT_USER
+
+static void
+read_cb(evutil_socket_t fd, short event, void *arg)
+{
+	char buf[16];
+	ev_ssize_t n;
+
+	if (EV_TIMEOUT & event) {
+		printf("%s: Timeout!\n", __func__);
+		exit(1);
+	}
+
+	if ((EV_READ & event) == 0) {
+		printf("%s: expected EV_READ for pipe but got nothing\n", __func__);
+		exit(1);
+	}
+
+	n = read(fd, buf, sizeof(buf));
+	if (n == -1) {
+		printf("%s: read error on pipe\n", __func__);
+		exit(1);
+	}
+	buf[n] = '\0';
+	if (strcmp(buf, data) != 0) {
+		printf("%s: read unexpected data from pipe: %s\n", __func__, buf);
+		exit(1);
+	}
+	printf("%s: read the expected data from pipe successfully\n", __func__);
+	assert(read_called == 0);
+	read_called++;
+}
+
+static void*
+trigger_kq(void *arg)
+{
+	struct event_base *base = arg;
+	/* This function is called to notify the main thread
+	 * to scan for new events immediately by issuing a EVFILT_USER event.
+	 * We need to do it in a separate thread, otherwise it won't be issued.
+	 */
+	event_base_loopcontinue(base);
+	return NULL;
+}
+
+static void
+notify_cb(evutil_socket_t fd, short events, void *arg)
+{
+	/* To ensure that the EVFILT_USER event is issued,
+	 * we need to do it in outside the main thread.
+	 */
+	pthread_t trigger;
+	pthread_create(&trigger, NULL, trigger_kq, arg);
+	pthread_join(trigger, NULL);
+}
+
+static void
+write_cb(evutil_socket_t fd, short events, void *arg)
+{
+	int *wfd = arg;
+	/* Write the data to the pipe */
+	if (write(*wfd, data, strlen(data)+1) == -1) {
+		printf("%s: write data to pipe error\n", __func__);
+		exit(1);
+	}
+	printf("%s: write data to pipe successfully\n", __func__);
+}
+
+static void
+exit_cb(int sock, short what, void *arg)
+{
+    struct event_base *base = arg;
+    event_base_loopbreak(base);
+}
+
+int
+main(int argc, char **argv)
+{
+	struct event_base *base;
+	struct event_config *cfg;
+	const char **methods;
+	struct event *ev_notify, *ev_read, *ev_write, *ev_exit;
+	struct timeval tv_notify, tv_write, tv_exit;
+	int pipefd[2];
+
+	/* Create a pair of pipe */
+	int r;
+	do {
+		r = pipe(pipefd);
+		if (r == -1) {
+			printf("pipe error\n");
+			return EXIT_FAILURE;
+		}
+		if (pipefd[0] != MAGIC_FD && pipefd[1] != MAGIC_FD)
+			break;
+		close(pipefd[0]);
+		close(pipefd[1]);
+		r = -1;
+	} while (r != 0);
+
+	/* Redirect the read end of the pipe to the magic number of EVFILT_USER,
+	 * verifying that the EVFILT_READ event is not tampered by the EVFILT_USER event.
+	 */
+	if (dup2(pipefd[0], MAGIC_FD) == -1) {
+		printf("dup2 failed\n");
+		return EXIT_FAILURE;
+	}
+	close(pipefd[0]);
+	pipefd[0] = MAGIC_FD;
+
+	/* Sets up Libevent for use with Pthreads locking and thread ID functions.
+	 * This is required for event_base_loopcontinue() to work properly.
+	 */
+	evthread_use_pthreads();
+
+	cfg = event_config_new();
+	methods = event_get_supported_methods();
+	for (size_t i = 0; methods[i] != NULL; ++i) {
+		if (strcmp(methods[i], "kqueue"))
+			event_config_avoid_method(cfg, methods[i]);
+	}
+	base = event_base_new_with_config(cfg);
+	event_config_free(cfg);
+
+	/* Triggering a EVFILT_USER event is expected to not tamper EVFILT_READ on the same indent. */
+	ev_notify = evtimer_new(base, notify_cb, base);
+	tv_notify.tv_sec = 0;
+	tv_notify.tv_usec = 0;
+	evtimer_add(ev_notify, &tv_notify);
+	ev_write = evtimer_new(base, write_cb, &pipefd[1]);
+	tv_write.tv_sec = 1;
+	tv_write.tv_usec = 0;
+	evtimer_add(ev_write, &tv_write);
+	ev_exit = evtimer_new(base, exit_cb, base);
+	tv_exit.tv_sec = 5; // exit after 5 seconds, after the timeout.
+	tv_exit.tv_usec = 0;
+	evtimer_add(ev_exit, &tv_exit);
+
+	/* Start dispatching events */
+	ev_read = event_new(base, MAGIC_FD, EV_READ | EV_TIMEOUT, read_cb, event_self_cbarg());
+	event_add(ev_read, &timeout);
+	event_base_dispatch(base);
+
+	// The read_cb is expected to be called once.
+	assert(read_called == 1);
+
+	/* Clean up the resources */
+	event_free(ev_read);
+	event_free(ev_notify);
+	event_free(ev_write);
+	event_free(ev_exit);
+	close(pipefd[0]);
+	close(pipefd[1]);
+	event_base_free(base);
+	return EXIT_SUCCESS;
+}

test/test.sh

@@ -2,6 +2,7 @@
 
 BACKENDS="EVPORT KQUEUE EPOLL DEVPOLL POLL SELECT WIN32 WEPOLL"
 TESTS="test-eof test-closed test-weof test-time test-changelist test-fdleak"
+KQUEUE_TESTS="test-kq-collision"
 FAILED=no
 TEST_OUTPUT_FILE=${TEST_OUTPUT_FILE:-/dev/null}
 REGRESS_ARGS=${REGRESS_ARGS:-}
@@ -65,14 +66,22 @@ announce_n () {
 
 
 run_tests () {
+	backend="$1" && shift
+	ALL_TESTS="$TESTS"
+
 	if $TEST_DIR/test-init 2>>"$TEST_OUTPUT_FILE" ;
 	then
-		true
+		announce "Running $backend $*"
 	else
-		announce Skipping test
+		announce "Skipping test $backend $*"
 		return
 	fi
-	for i in $TESTS; do
+
+	if [ "$backend" = "KQUEUE" ]; then
+		ALL_TESTS="$ALL_TESTS $KQUEUE_TESTS"
+	fi
+
+	for i in $ALL_TESTS; do
 		announce_n " $i: "
 		if $TEST_DIR/$i >>"$TEST_OUTPUT_FILE" ;
 		then
@@ -132,21 +141,27 @@ run_tests () {
 }
 
 do_test() {
+	backend="$1" && shift
+	if [ $# -gt 1 ]; then
+		backend_conf="$2" && shift
+	else
+		backend_conf=""
+	fi
+
 	setup
-	announce "$1 $2"
-	unset EVENT_NO$1
-	if test "$2" = "(changelist)" ; then
+	unset EVENT_NO$backend
+	if test "$backend_conf" = "(changelist)" ; then
 	    EVENT_EPOLL_USE_CHANGELIST=yes; export EVENT_EPOLL_USE_CHANGELIST
-	elif test "$2" = "(timerfd)" ; then
+	elif test "$backend_conf" = "(timerfd)" ; then
 	    EVENT_PRECISE_TIMER=1; export EVENT_PRECISE_TIMER
-	elif test "$2" = "(signalfd)" ; then
+	elif test "$backend_conf" = "(signalfd)" ; then
 	    EVENT_USE_SIGNALFD=1; export EVENT_USE_SIGNALFD
-	elif test "$2" = "(timerfd+changelist)" ; then
+	elif test "$backend_conf" = "(timerfd+changelist)" ; then
 	    EVENT_EPOLL_USE_CHANGELIST=yes; export EVENT_EPOLL_USE_CHANGELIST
 	    EVENT_PRECISE_TIMER=1; export EVENT_PRECISE_TIMER
 	fi
 
-	run_tests
+	run_tests "$backend" "$backend_conf"
 }
 
 usage()
@@ -178,6 +193,8 @@ main()
 		esac
 	done
 
+	set -e
+
 	announce "Running tests:"
 
 	[ $timerfd -eq 0 ] || do_test EPOLL "(timerfd)"