From b0f31f578b0669b598d3ded3a625685b125bef1d Mon Sep 17 00:00:00 2001 From: Garrett D'Amore Date: Tue, 3 Oct 2017 20:28:09 -0700 Subject: Improve UDP test coverage, fix numerous issues found. We introduced richer, deeper tests for UDP functionality. These tests uncovered a number of issues which this commit fixes. The Windows IOCP code needs to support multiple aios on a single nni_win_event. A redesign of the IOCP handling addresses that. The POSIX UDP code also needed fixes; foremost among them is the fact that the UDP file descriptor is not placed into non-blocking mode, leading to potential hangs. A number of race conditions and bugs along the implementation of the above items were uncovered and fixed. To the best of our knowledge the current code is bug-free. --- src/platform/windows/win_debug.c | 13 ++-- src/platform/windows/win_impl.h | 4 +- src/platform/windows/win_iocp.c | 125 +++++++++++++++++++++++------------- src/platform/windows/win_sockaddr.c | 6 ++ 4 files changed, 99 insertions(+), 49 deletions(-) (limited to 'src/platform/windows') diff --git a/src/platform/windows/win_debug.c b/src/platform/windows/win_debug.c index 5c6c3fb5..0113af58 100644 --- a/src/platform/windows/win_debug.c +++ b/src/platform/windows/win_debug.c @@ -83,8 +83,9 @@ nni_plat_errno(int errnum) static struct { int win_err; int nng_err; -} nni_win_errnos[] = { - // clang-format off +} nni_win_errnos[] = + { + // clang-format off { ERROR_FILE_NOT_FOUND, NNG_ENOENT }, { ERROR_ACCESS_DENIED, NNG_EPERM }, { ERROR_INVALID_HANDLE, NNG_ECLOSED }, @@ -114,6 +115,10 @@ static struct { { WSAENOPROTOOPT, NNG_ENOTSUP }, { WSAEPROTONOSUPPORT, NNG_ENOTSUP }, { WSAEPROTONOSUPPORT, NNG_ENOTSUP }, + { WSAESOCKTNOSUPPORT, NNG_ENOTSUP }, + { WSAEOPNOTSUPP, NNG_ENOTSUP }, + { WSAEPFNOSUPPORT, NNG_ENOTSUP }, + { WSAEAFNOSUPPORT, NNG_ENOTSUP }, { WSAEADDRINUSE, NNG_EADDRINUSE }, { WSAEADDRNOTAVAIL, NNG_EADDRINVAL }, { WSAENETDOWN, NNG_EUNREACHABLE }, @@ -137,8 +142,8 @@ static struct { // Must be Last!! { 0, 0 }, - // clang-format on -}; + // clang-format on + }; // This converts a Windows API error (from GetLastError()) to an // nng standard error code. diff --git a/src/platform/windows/win_impl.h b/src/platform/windows/win_impl.h index c2549266..236feb31 100644 --- a/src/platform/windows/win_impl.h +++ b/src/platform/windows/win_impl.h @@ -63,13 +63,15 @@ struct nni_win_event_ops { struct nni_win_event { OVERLAPPED olpd; void * ptr; - nni_aio * aio; nni_mtx mtx; nni_cv cv; unsigned run : 1; unsigned fini : 1; + unsigned closed : 1; unsigned count; int status; + nni_list aios; + nni_aio * active; nni_win_event_ops ops; }; diff --git a/src/platform/windows/win_iocp.c b/src/platform/windows/win_iocp.c index 6d2438d3..0f9348d6 100644 --- a/src/platform/windows/win_iocp.c +++ b/src/platform/windows/win_iocp.c @@ -24,11 +24,16 @@ static HANDLE nni_win_global_iocp = NULL; static nni_thr nni_win_iocp_thrs[NNI_WIN_IOCP_NTHREADS]; static nni_mtx nni_win_iocp_mtx; +static void nni_win_event_start(nni_win_event *); + static void -nni_win_event_finish(nni_win_event *evt, nni_aio *aio) +nni_win_event_finish(nni_win_event *evt) { + nni_aio *aio; evt->run = 0; - if (aio != NULL) { + + if ((aio = evt->active) != NULL) { + evt->active = NULL; evt->ops.wev_finish(evt, aio); } if (evt->fini) { @@ -44,9 +49,7 @@ nni_win_iocp_handler(void *arg) ULONG_PTR key; OVERLAPPED * olpd; nni_win_event *evt; - int rv; BOOL ok; - nni_aio * aio; NNI_ARG_UNUSED(arg); @@ -70,16 +73,15 @@ nni_win_iocp_handler(void *arg) nni_mtx_lock(&evt->mtx); if (ok) { - rv = ERROR_SUCCESS; + evt->status = 0; } else if (evt->status == 0) { evt->status = nni_win_error(GetLastError()); } - aio = evt->aio; - evt->aio = NULL; evt->count = cnt; - nni_win_event_finish(evt, aio); + nni_win_event_finish(evt); + nni_win_event_start(evt); nni_mtx_unlock(&evt->mtx); } } @@ -90,47 +92,65 @@ nni_win_event_cancel(nni_aio *aio, int rv) nni_win_event *evt = aio->a_prov_data; nni_mtx_lock(&evt->mtx); - if (evt->aio == aio) { + if (aio == evt->active) { evt->status = rv; // Use provider specific cancellation. evt->ops.wev_cancel(evt); + } else if (nni_aio_list_active(aio)) { + nni_aio_list_remove(aio); + nni_aio_finish_error(aio, rv); } nni_mtx_unlock(&evt->mtx); } void -nni_win_event_resubmit(nni_win_event *evt, nni_aio *aio) +nni_win_event_start(nni_win_event *evt) { - // This is just continuation of a pre-existing AIO operation. - // For example, continuing I/O of a multi-buffer s/g operation. - // The lock is held. + nni_aio *aio; + + // Lock held. + + if (evt->run) { + // Already running. + return; + } // Abort operation -- no further activity. - if (evt->fini) { - evt->run = 0; - nni_cv_wake(&evt->cv); + if (evt->fini || evt->closed) { + while ((aio = nni_list_first(&evt->aios)) != NULL) { + nni_aio_list_remove(aio); + nni_aio_finish_error(aio, NNG_ECLOSED); + } return; } + if ((aio = nni_list_first(&evt->aios)) == NULL) { + return; + } + + nni_aio_list_remove(aio); + evt->active = aio; evt->status = 0; evt->count = 0; if (!ResetEvent(evt->olpd.hEvent)) { - evt->status = nni_win_error(GetLastError()); - evt->count = 0; - nni_win_event_finish(evt, aio); + evt->active = NULL; + nni_aio_finish_error(aio, nni_win_error(GetLastError())); return; } - evt->aio = aio; evt->run = 1; if (evt->ops.wev_start(evt, aio) != 0) { // Start completed synchronously. It will have stored // the count and status in the evt. - evt->aio = NULL; - nni_win_event_finish(evt, aio); + nni_win_event_finish(evt); } } +void +nni_win_event_resubmit(nni_win_event *evt, nni_aio *aio) +{ + nni_aio_list_prepend(&evt->aios, aio); +} void nni_win_event_submit(nni_win_event *evt, nni_aio *aio) @@ -141,7 +161,8 @@ nni_win_event_submit(nni_win_event *evt, nni_aio *aio) nni_mtx_unlock(&evt->mtx); return; } - nni_win_event_resubmit(evt, aio); + nni_aio_list_append(&evt->aios, aio); + nni_win_event_start(evt); nni_mtx_unlock(&evt->mtx); } @@ -154,12 +175,20 @@ nni_win_event_complete(nni_win_event *evt, int cnt) void nni_win_event_close(nni_win_event *evt) { - if (evt->ptr != NULL) { - nni_mtx_lock(&evt->mtx); - evt->status = NNG_ECLOSED; - evt->ops.wev_cancel(evt); - nni_mtx_unlock(&evt->mtx); + nni_aio *aio; + + if (evt->ptr == NULL) { + return; // Never initialized + } + nni_mtx_lock(&evt->mtx); + evt->closed = 1; + evt->status = NNG_ECLOSED; + evt->ops.wev_cancel(evt); + while ((aio = nni_list_first(&evt->aios)) != NULL) { + nni_aio_list_remove(aio); + nni_aio_finish_error(aio, NNG_ECLOSED); } + nni_mtx_unlock(&evt->mtx); } int @@ -175,44 +204,52 @@ int nni_win_event_init(nni_win_event *evt, nni_win_event_ops *ops, void *ptr) { ZeroMemory(&evt->olpd, sizeof(evt->olpd)); - evt->olpd.hEvent = CreateEvent(NULL, TRUE, TRUE, NULL); - if (evt->olpd.hEvent == NULL) { - return (nni_win_error(GetLastError())); - } nni_mtx_init(&evt->mtx); nni_cv_init(&evt->cv, &evt->mtx); - + nni_aio_list_init(&evt->aios); evt->ops = *ops; - evt->aio = NULL; evt->ptr = ptr; evt->fini = 0; evt->run = 0; + + evt->olpd.hEvent = CreateEvent(NULL, TRUE, TRUE, NULL); + if (evt->olpd.hEvent == NULL) { + return (nni_win_error(GetLastError())); + } + return (0); } void nni_win_event_fini(nni_win_event *evt) { - if (evt->ptr != NULL) { - nni_mtx_lock(&evt->mtx); + nni_aio *aio; - evt->fini = 1; + if (evt->ptr == NULL) { + return; // Never initialized + } + nni_mtx_lock(&evt->mtx); - // Use provider specific cancellation. - evt->ops.wev_cancel(evt); + evt->fini = 1; - // Wait for everything to stop referencing this. - while (evt->run) { - nni_cv_wait(&evt->cv); - } + // Use provider specific cancellation. + evt->ops.wev_cancel(evt); - nni_mtx_unlock(&evt->mtx); + // Wait for everything to stop referencing this. + while (evt->run) { + nni_cv_wait(&evt->cv); + } + + while ((aio = nni_list_first(&evt->aios)) != NULL) { + nni_aio_list_remove(aio); + nni_aio_finish_error(aio, NNG_ECLOSED); } if (evt->olpd.hEvent != NULL) { (void) CloseHandle(evt->olpd.hEvent); evt->olpd.hEvent = NULL; } + nni_mtx_unlock(&evt->mtx); nni_cv_fini(&evt->cv); nni_mtx_fini(&evt->mtx); } diff --git a/src/platform/windows/win_sockaddr.c b/src/platform/windows/win_sockaddr.c index 0fa6dd51..f66542c6 100644 --- a/src/platform/windows/win_sockaddr.c +++ b/src/platform/windows/win_sockaddr.c @@ -20,6 +20,9 @@ nni_win_nn2sockaddr(SOCKADDR_STORAGE *ss, const nni_sockaddr *sa) SOCKADDR_IN * sin; SOCKADDR_IN6 *sin6; + if ((ss == NULL) || (sa == NULL)) { + return (-1); + } switch (sa->s_un.s_family) { case NNG_AF_INET: sin = (void *) ss; @@ -46,6 +49,9 @@ nni_win_sockaddr2nn(nni_sockaddr *sa, const SOCKADDR_STORAGE *ss) SOCKADDR_IN * sin; SOCKADDR_IN6 *sin6; + if ((ss == NULL) || (sa == NULL)) { + return (-1); + } switch (ss->ss_family) { case PF_INET: sin = (void *) ss; -- cgit v1.2.3-70-g09d2