From 3ca7bcc0edd0f26c33264d32e7b6f07276e72e3c Mon Sep 17 00:00:00 2001 From: Garrett D'Amore Date: Sat, 7 Dec 2024 19:32:40 -0800 Subject: [PATCH] aio: task_abort was a mistake The use of task_abort to prematurely fail an aio at scheduling time was a mistake, because it could have led to duplicate calls to nng_aio_finish(). We do need to ensure that we leave an indicator so that nni_aio_schedule can return the abort status to caller, in the case that abort is called between the nni_aio_begin and nni_aio_schedule calls. --- include/nng/nng.h | 3 +++ src/core/aio.c | 22 ++++++++++++---------- src/core/aio.h | 1 + src/core/taskq.c | 14 -------------- src/core/taskq.h | 5 ----- 5 files changed, 16 insertions(+), 29 deletions(-) diff --git a/include/nng/nng.h b/include/nng/nng.h index 3397ca536..0ce5f46cc 100644 --- a/include/nng/nng.h +++ b/include/nng/nng.h @@ -543,6 +543,8 @@ NNG_DECL void nng_aio_reap(nng_aio *); // AIO to be free, including for the callback to have completed // execution. Therefore, the caller must NOT hold any locks that // are acquired in the callback, or deadlock will occur. +// No further operations may be scheduled on the aio, stop is +// a permanent operation. NNG_DECL void nng_aio_stop(nng_aio *); // nng_aio_result returns the status/result of the operation. This @@ -558,6 +560,7 @@ NNG_DECL size_t nng_aio_count(nng_aio *); // nng_aio_cancel attempts to cancel any in-progress I/O operation. // The AIO callback will still be executed, but if the cancellation is // successful then the status will be NNG_ECANCELED. +// An AIO can only be canceled if it was submitted already. NNG_DECL void nng_aio_cancel(nng_aio *); // nng_aio_abort is like nng_aio_cancel, but allows for a different diff --git a/src/core/aio.c b/src/core/aio.c index d003c2517..eaff5c805 100644 --- a/src/core/aio.c +++ b/src/core/aio.c @@ -118,8 +118,6 @@ nni_aio_fini(nni_aio *aio) if (fn != NULL) { fn(aio, arg, NNG_ECLOSED); - } else { - nni_task_abort(&aio->a_task); } nni_task_fini(&aio->a_task); @@ -203,8 +201,6 @@ nni_aio_stop(nni_aio *aio) if (fn != NULL) { fn(aio, arg, NNG_ECANCELED); - } else { - nni_task_abort(&aio->a_task); } nni_aio_wait(aio); @@ -230,8 +226,6 @@ nni_aio_close(nni_aio *aio) if (fn != NULL) { fn(aio, arg, NNG_ECLOSED); - } else { - nni_task_abort(&aio->a_task); } } } @@ -352,6 +346,7 @@ nni_aio_begin(nni_aio *aio) aio->a_result = 0; aio->a_count = 0; aio->a_cancel_fn = NULL; + aio->a_abort = false; // We should not reschedule anything at this point. if (aio->a_stop || eq->eq_stop) { @@ -378,7 +373,6 @@ nni_aio_schedule(nni_aio *aio, nni_aio_cancel_fn cancel, void *data) // Convert the relative timeout to an absolute timeout. switch (aio->a_timeout) { case NNG_DURATION_ZERO: - nni_task_abort(&aio->a_task); return (NNG_ETIMEDOUT); case NNG_DURATION_INFINITE: case NNG_DURATION_DEFAULT: @@ -391,8 +385,12 @@ nni_aio_schedule(nni_aio *aio, nni_aio_cancel_fn cancel, void *data) } nni_mtx_lock(&eq->eq_mtx); + if (aio->a_abort) { + int rv = aio->a_result; + nni_mtx_unlock(&eq->eq_mtx); + return (rv); + } if (aio->a_stop || eq->eq_stop) { - nni_task_abort(&aio->a_task); nni_mtx_unlock(&eq->eq_mtx); return (NNG_ECLOSED); } @@ -426,13 +424,17 @@ nni_aio_abort(nni_aio *aio, int rv) arg = aio->a_cancel_arg; aio->a_cancel_fn = NULL; aio->a_cancel_arg = NULL; + if (fn == NULL) { + // We haven't been scheduled yet, + // so make sure that schedule will abort. + aio->a_abort = true; + aio->a_result = rv; + } nni_mtx_unlock(&eq->eq_mtx); // Stop any I/O at the provider level. if (fn != NULL) { fn(aio, arg, rv); - } else { - nni_task_abort(&aio->a_task); } } } diff --git a/src/core/aio.h b/src/core/aio.h index 5f7ddb695..9491a2fa9 100644 --- a/src/core/aio.h +++ b/src/core/aio.h @@ -214,6 +214,7 @@ struct nng_aio { bool a_expire_ok; // Expire from sleep is ok bool a_expiring; // Expiration in progress bool a_use_expire; // Use expire instead of timeout + bool a_abort; // Task was aborted bool a_init; // Initialized this nni_task a_task; diff --git a/src/core/taskq.c b/src/core/taskq.c index 1f0ae1b6b..53252a8e7 100644 --- a/src/core/taskq.c +++ b/src/core/taskq.c @@ -198,20 +198,6 @@ nni_task_prep(nni_task *task) nni_mtx_unlock(&task->task_mtx); } -void -nni_task_abort(nni_task *task) -{ - // This is called when unscheduling the task. - nni_mtx_lock(&task->task_mtx); - if (task->task_prep) { - task->task_prep = false; - task->task_busy--; - if (task->task_busy == 0) { - nni_cv_wake(&task->task_cv); - } - } - nni_mtx_unlock(&task->task_mtx); -} void nni_task_wait(nni_task *task) { diff --git a/src/core/taskq.h b/src/core/taskq.h index d32994334..356ef725e 100644 --- a/src/core/taskq.h +++ b/src/core/taskq.h @@ -40,11 +40,6 @@ extern void nni_task_exec(nni_task *); // nni_task_exec). extern void nni_task_prep(nni_task *); -// nni_task_abort is called to undo the effect of nni_task_prep, -// basically. The aio framework uses this when nni_aio_schedule() -// returns an error. -extern void nni_task_abort(nni_task *); - // nni_task_busy checks to see if a task is still busy. // This is uses the same check that nni_task_wait uses. extern bool nni_task_busy(nni_task *);