From dc878e3098b189e635ca003e80f18b5df6883007 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sat, 29 Oct 2022 18:05:39 -0700 Subject: [PATCH] isc_async_run() runs events in reverse order when more than one event was scheduled in the isc_aysnc queue, they were executed in reverse order. we need to pull events off the back of queue instead the front, so that uv_loop will run them in the right order. note that isc_job_run() has the same behavior, because it calls uv_idle_start() directly. in that case we just document it so it'll be less surprising in the future. --- lib/isc/async.c | 4 +++- lib/isc/include/isc/job.h | 5 +++++ lib/isc/loop.c | 9 +++++++-- tests/isc/async_test.c | 39 ++++++++++++++++++++++++++++++++++++--- tests/isc/job_test.c | 31 +++++++++++++++++++++++++++++++ 5 files changed, 82 insertions(+), 6 deletions(-) diff --git a/lib/isc/async.c b/lib/isc/async.c index 03e2cbd799..a4d08eaadf 100644 --- a/lib/isc/async.c +++ b/lib/isc/async.c @@ -47,7 +47,9 @@ isc_async_run(isc_loop_t *loop, isc_job_cb cb, void *cbarg) { job = isc__job_new(loop, cb, cbarg); - /* Now send the half initialized job to loop queue */ + /* + * Now send the half-initialized job to the loop queue. + */ LOCK(&loop->queue_lock); ISC_LIST_APPEND(loop->queue_jobs, job, link); UNLOCK(&loop->queue_lock); diff --git a/lib/isc/include/isc/job.h b/lib/isc/include/isc/job.h index 2c75e3042c..19ab75d724 100644 --- a/lib/isc/include/isc/job.h +++ b/lib/isc/include/isc/job.h @@ -39,6 +39,11 @@ isc_job_run(isc_loopmgr_t *loopmgr, isc_job_cb cb, void *cbarg); * Schedule the job callback 'cb' to be run on the currently * running event loop. * + * Note: Because of the design of uv_idle_start(), if more than one + * job is posted at once, the jobs will be pushed onto a stack and + * executed in last-in-first-out order. To post events that are + * executed in order posted, use isc_async_run() instead. + * * Requires: * *\li 'loopmgr' is the active loop manager. diff --git a/lib/isc/loop.c b/lib/isc/loop.c index 52817be01c..b77ba534f8 100644 --- a/lib/isc/loop.c +++ b/lib/isc/loop.c @@ -191,9 +191,14 @@ queue_cb(uv_async_t *handle) { ISC_LIST_MOVE(list, loop->queue_jobs); UNLOCK(&loop->queue_lock); - job = ISC_LIST_HEAD(list); + /* + * The ISC_LIST_TAIL is counterintuitive here, but uv_idle + * drains its queue backwards, so if there's more than one event to + * be processed then they need to be in reverse order. + */ + job = ISC_LIST_TAIL(list); while (job != NULL) { - isc_job_t *next = ISC_LIST_NEXT(job, link); + isc_job_t *next = ISC_LIST_PREV(job, link); ISC_LIST_UNLINK(list, job, link); isc__job_init(loop, job); diff --git a/tests/isc/async_test.c b/tests/isc/async_test.c index 138147b3e0..41f0ee18f5 100644 --- a/tests/isc/async_test.c +++ b/tests/isc/async_test.c @@ -39,9 +39,10 @@ static atomic_uint scheduled = 0; static void async_cb(void *arg) { - UNUSED(arg); uint32_t tid = isc_tid(); + UNUSED(arg); + atomic_fetch_add(&scheduled, 1); if (tid > 0) { @@ -54,11 +55,11 @@ async_cb(void *arg) { static void async_setup_cb(void *arg) { - UNUSED(arg); uint32_t tid = isc_loopmgr_nloops(loopmgr) - 1; - isc_loop_t *loop = isc_loop_get(loopmgr, tid); + UNUSED(arg); + isc_async_run(loop, async_cb, loopmgr); } @@ -68,8 +69,40 @@ ISC_RUN_TEST_IMPL(isc_async_run) { assert_int_equal(atomic_load(&scheduled), loopmgr->nloops); } +static char string[32] = ""; +int n1 = 1, n2 = 2, n3 = 3, n4 = 4, n5 = 5; + +static void +append(void *arg) { + char value[32]; + sprintf(value, "%d", *(int *)arg); + strlcat(string, value, 10); +} + +static void +async_multiple(void *arg) { + isc_loop_t *loop = isc_loop_current(loopmgr); + + UNUSED(arg); + + isc_async_run(loop, append, &n1); + isc_async_run(loop, append, &n2); + isc_async_run(loop, append, &n3); + isc_async_run(loop, append, &n4); + isc_async_run(loop, append, &n5); + isc_loopmgr_shutdown(loopmgr); +} + +ISC_RUN_TEST_IMPL(isc_async_multiple) { + string[0] = '\0'; + isc_loop_setup(isc_loop_main(loopmgr), async_multiple, loopmgr); + isc_loopmgr_run(loopmgr); + assert_string_equal(string, "12345"); +} + ISC_TEST_LIST_START ISC_TEST_ENTRY_CUSTOM(isc_async_run, setup_loopmgr, teardown_loopmgr) +ISC_TEST_ENTRY_CUSTOM(isc_async_multiple, setup_loopmgr, teardown_loopmgr) ISC_TEST_LIST_END ISC_TEST_MAIN diff --git a/tests/isc/job_test.c b/tests/isc/job_test.c index 4ac4c0a903..2a8c049bed 100644 --- a/tests/isc/job_test.c +++ b/tests/isc/job_test.c @@ -76,8 +76,39 @@ ISC_RUN_TEST_IMPL(isc_job_run) { assert_int_equal(atomic_load(&scheduled), atomic_load(&executed)); } +static char string[32] = ""; +int n1 = 1, n2 = 2, n3 = 3, n4 = 4, n5 = 5; + +static void +append(void *arg) { + char value[32]; + sprintf(value, "%d", *(int *)arg); + strlcat(string, value, 10); +} + +static void +job_multiple(void *arg) { + UNUSED(arg); + + /* These will be processed in reverse order */ + isc_job_run(loopmgr, append, &n1); + isc_job_run(loopmgr, append, &n2); + isc_job_run(loopmgr, append, &n3); + isc_job_run(loopmgr, append, &n4); + isc_job_run(loopmgr, append, &n5); + isc_loopmgr_shutdown(loopmgr); +} + +ISC_RUN_TEST_IMPL(isc_job_multiple) { + string[0] = '\0'; + isc_loop_setup(isc_loop_main(loopmgr), job_multiple, loopmgr); + isc_loopmgr_run(loopmgr); + assert_string_equal(string, "54321"); +} + ISC_TEST_LIST_START ISC_TEST_ENTRY_CUSTOM(isc_job_run, setup_loopmgr, teardown_loopmgr) +ISC_TEST_ENTRY_CUSTOM(isc_job_multiple, setup_loopmgr, teardown_loopmgr) ISC_TEST_LIST_END ISC_TEST_MAIN