From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 186506E459; Fri, 19 Nov 2021 13:30:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 186506E459 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1637317817; bh=c5HnxVmP9j01ElHLbwYoaUpfoXX8wcIgHIrMPEmSj70=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=QKedQZ7GNRRuhg6CiGlrHvqIql0+KHdcJDnhjGo47QIMVGu51BI0QodBI80cuR/CY lTZELIyIh1bskPoGlEKNdYd+n8IVyiN/0+IG1AO+7I0t+6m1m2Ql8FPrODuyxL2yjR oKRb3XmstRNstjqCZadTnxnB8FYLHdHXIeo4VwK4= Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C7E2D6E459 for ; Fri, 19 Nov 2021 13:30:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C7E2D6E459 Received: by smtp59.i.mail.ru with esmtpa (envelope-from ) id 1mo19q-0003kG-Ip; Fri, 19 Nov 2021 13:30:15 +0300 Message-ID: <73c8ec91-322f-7f67-c822-e5409ce2e999@tarantool.org> Date: Fri, 19 Nov 2021 13:30:14 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.3.1 To: Vladislav Shpilevoy , vdavydov@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <20211116101731.48597-1-sergepetrenko@tarantool.org> <07bdcdec-8dbc-35f2-6b76-b5a432f97441@tarantool.org> <1046a3bb-cda1-f64a-d172-71e0022895a6@tarantool.org> Content-Language: en-GB In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9731B3922EC0639791BFBC8C450C80827E49496A0B58C59AD00894C459B0CD1B987A210117226F0ADDA453EB7DA91C5FC4C98EEFDB974FA25A24CDA786A8AA87E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7011EB7026DD4A9BAEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063725D748B084CAA27D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8FED5CB98BB668A09EEBEE5861C597ADB117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751F28451B159A507268D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE7B96B19DC40933219935A1E27F592749D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3A6C7FFFE744CA7FB6E0066C2D8992A16C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F790063757B1FBEA53BC6EDBEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5194A3185E8DBCF5CB7C30BE36183EB072BC3843D2952D3E2D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7506FE1F977233B9BB410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34AB80C45F81B80D634E74BF5D7BC386A81FA8E677E9FC02F06506B594FAF76CB69F22922D58F918E61D7E09C32AA3244C48FD8F24ED0D9270C14493A4757FEE028894E9C85370243EFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojUkwcpHt8ZEcQ0yjWyNiE5w== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE407BB37203509E7CAC43AD4FFB4243FE22B4B4703A72C807C0F6BB2E709EA627F343C7DDD459B58856F0E45BC603594F5A135B915D4279FF0579437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] coio: handle spurious wakeup correctly X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 18.11.2021 02:11, Vladislav Shpilevoy пишет: >>> On 16.11.2021 11:17, Serge Petrenko via Tarantool-patches wrote: >>>> coio_accept, coio_read, coio_write, coio_writev used to handle spurious >>>> wakeups correctly: if the timeout hasn't passed yet, they would simply >>>> retry reading (or writing) and fall asleep once again if no data is >>>> ready. >>>> >>>> This behaviour changed in the following patches: >>>> 577a640a7fdec986d19101ed04d2afa80e951c78 ("coio: pass fd to >>>> coio_accept") and 4f84859dcdd6126b0bdcda810b7f5f58386bdac6 ("Introduce >>>> iostream wrapper for socket I/O"). >>>> >>>> Now the functions timeout on the very first spurious wakeup. >>>> >>>> Fix this, add the appropriate unit tests and a test_iostream >>>> implementation for the ease of testing. >>> Don't we have the same problem with coio_connect_addr() (used in >>> coio_connect_timeout())? >> Not really. Neither coio_connect_addr() nor coio_connect_timeout() retry >> the connection. So even the previous version would throw an error after >> a spurious wakeup. Just the error would be different. >> Before the change it would throw SocketError, not TimedOut, but I don't think >> this matters much. Does it? >> >> By the change I mean (2db0741b) "coio: return fd from coio_connect". > Ok, then never mind. I will wait for fixes of Vova's comments regarding the > tests, but the bugfix itself LGTM. Thanks for your answer! Here's the incremental diff after the fixes. Long story short, I've deleted the test_iostream wrapper. There wasn't much use for it. I've force-pushed the new version on the branch. =============================== diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt index 71c8ba981..d8b1aac0d 100644 --- a/test/unit/CMakeLists.txt +++ b/test/unit/CMakeLists.txt @@ -121,7 +121,7 @@ if (GLIBC_USED)      target_link_libraries(cbus_hang.test core unit stat)  endif () -add_executable(coio.test coio.cc core_test_utils.c test_iostream.c) +add_executable(coio.test coio.cc core_test_utils.c)  target_link_libraries(coio.test core eio bit uri unit)  if (ENABLE_BUNDLED_MSGPUCK) diff --git a/test/unit/coio.cc b/test/unit/coio.cc index 194503fda..d55bc2347 100644 --- a/test/unit/coio.cc +++ b/test/unit/coio.cc @@ -4,7 +4,11 @@  #include "coio_task.h"  #include "fio.h"  #include "unit.h" -#include "test_iostream.h" +#include "iostream.h" + +#include +#include +#include  int  touch_f(va_list ap) @@ -137,7 +141,7 @@ static int  test_read_f(va_list ap)  {      struct iostream *io = va_arg(ap, struct iostream *); -    char buf[5]; +    char buf[1024];      int rc = coio_read(io, buf, sizeof(buf));      if (rc < (ssize_t)sizeof(buf))          return -1; @@ -148,9 +152,9 @@ static int  test_write_f(va_list ap)  {      struct iostream *io = va_arg(ap, struct iostream *); -    const char str[] = "test"; -    int rc = coio_write_timeout(io, str, sizeof(str), TIMEOUT_INFINITY); -    if (rc < (ssize_t)sizeof(str)) +    char buf[1024] = ""; +    int rc = coio_write_timeout(io, buf, sizeof(buf), TIMEOUT_INFINITY); +    if (rc < (ssize_t)sizeof(buf))          return -1;      return 0;  } @@ -159,14 +163,45 @@ static int  test_writev_f(va_list ap)  {      struct iostream *io = va_arg(ap, struct iostream *); -    const char str[] = "test"; -    struct iovec iov = {(void *)str, sizeof(str)}; +    char buf[1024] = ""; +    struct iovec iov = {(void *)buf, sizeof(buf)};      int rc = coio_writev(io, &iov, 1, 0); -    if (rc < (ssize_t)sizeof(str)) +    if (rc < (ssize_t)sizeof(buf))          return -1;      return 0;  } +static void +fill_pipe(int fd) +{ +    char buf[1024] = ""; +    int rc = 0; +    while (rc >= 0 || errno == EINTR) +        rc = write(fd, buf, sizeof(buf)); +    fail_unless(errno == EAGAIN || errno == EWOULDBLOCK); +} + +static void +empty_pipe(int fd) +{ +    char buf[1024]; +    int rc = 0; +    while (rc >= 0 || errno == EINTR) +        rc = read(fd, buf, sizeof(buf)); +    fail_unless(errno == EAGAIN || errno == EWOULDBLOCK); +} + +static void +create_pipe(int fds[2]) +{ +    int rc = pipe(fds); +    fail_unless(rc >= 0); +    rc = fcntl(fds[0], F_SETFL, O_NONBLOCK); +    fail_unless(rc >= 0); +    rc = fcntl(fds[1], F_SETFL, O_NONBLOCK); +    fail_unless(rc >= 0); +} +  static void  read_write_test(void)  { @@ -186,25 +221,36 @@ read_write_test(void)      int num_tests = sizeof(test_funcs) / sizeof(test_funcs[0]);      plan(2 * num_tests); -    struct test_stream s; -    test_stream_create(&s, 0, 0); +    int fds[2]; +    create_pipe(fds);      for (int i = 0; i < num_tests; i++) { -        test_stream_reset(&s, 0, 0); +        struct iostream io; +        if (i == 0) { +            /* A non-readable fd, since the pipe is empty. */ +            iostream_create(&io, fds[0]); +        } else { +            iostream_create(&io, fds[1]); +            /* Make the fd non-writable. */ +            fill_pipe(fds[1]); +        }          struct fiber *f = fiber_new_xc("rw_test", test_funcs[i]);          fiber_set_joinable(f, true); -        fiber_start(f, &s.io); +        fiber_start(f, &io);          fiber_wakeup(f);          fiber_sleep(0);          ok(!fiber_is_dead(f), "coio_%s handle spurious wakeup",             descr[i]); -        test_stream_reset(&s, INT_MAX, INT_MAX); -        fiber_wakeup(f); +        if (i == 0) +            fill_pipe(fds[1]); +        else +            empty_pipe(fds[0]);          int rc = fiber_join(f);          ok(rc == 0, "coio_%s success after a spurious wakeup",             descr[i]); +        iostream_destroy(&io);      } -    test_stream_destroy(&s); - +    close(fds[0]); +    close(fds[1]);      check_plan();      footer();  } diff --git a/test/unit/test_iostream.c b/test/unit/test_iostream.c deleted file mode 100644 index c50b63944..000000000 --- a/test/unit/test_iostream.c +++ /dev/null @@ -1,125 +0,0 @@ -/* - * SPDX-License-Identifier: BSD-2-Clause - * - * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file. - */ - -#include -#include -#include - -#include "test_iostream.h" -#include "trivia/util.h" - -static ssize_t -test_stream_read(struct iostream *io, void *buf, size_t count) -{ -    struct test_stream_ctx *ctx = io->ctx; -    io->fd = ctx->rfd; -    (void)buf; -    ssize_t nrd = 0; -    if (ctx->avail_rd == 0) -        return IOSTREAM_WANT_READ; -    nrd = MIN(count, ctx->avail_rd); -    ctx->avail_rd -= nrd; -    return nrd; -} - -static ssize_t -test_stream_write(struct iostream *io, const void *buf, size_t count) -{ -    assert(io->fd >= 0); -    struct test_stream_ctx *ctx = io->ctx; -    io->fd = ctx->wfd; -    (void)buf; -    ssize_t nwr = 0; -    if (ctx->avail_wr == 0) -        return IOSTREAM_WANT_WRITE; -    nwr = MIN(count, ctx->avail_wr); -    ctx->avail_wr -= nwr; -    return nwr; -} - -static ssize_t -test_stream_writev(struct iostream *io, const struct iovec *iov, int iovcnt) -{ -    assert(io->fd >= 0); -    struct test_stream_ctx *ctx = io->ctx; -    io->fd = ctx->wfd; -    if (ctx->avail_wr == 0) -        return IOSTREAM_WANT_WRITE; -    ssize_t start_wr = ctx->avail_wr; -    for (int i = 0; i < iovcnt && ctx->avail_wr > 0; i++) { -        size_t nwr = MIN(iov[i].iov_len, ctx->avail_wr); -        ctx->avail_wr -= nwr; -    } -    return start_wr - ctx->avail_wr; -} - -static void -test_stream_delete_ctx(void *ptr) -{ -    struct test_stream_ctx *ctx = ptr; -    close(ctx->rfds[0]); -    close(ctx->rfds[1]); -    close(ctx->wfds[0]); -    close(ctx->wfds[1]); -} - -static const struct iostream_vtab test_stream_vtab = { -    test_stream_delete_ctx, -    test_stream_read, -    test_stream_write, -    test_stream_writev, -}; - -static void -fill_pipe(int fd) -{ -    char buf[4096]; -    int rc = 0; -    rc = fcntl(fd, F_SETFL, O_NONBLOCK); -    assert(rc >= 0); -    while (rc >= 0 || errno == EINTR) -        rc = write(fd, buf, sizeof(buf)); -    assert(errno == EAGAIN || errno == EWOULDBLOCK); -} - -static void -test_stream_ctx_create(struct test_stream_ctx *ctx, size_t maxrd, size_t maxwr) -{ -    ctx->avail_rd = maxrd; -    ctx->avail_wr = maxwr; -    int rc = pipe(ctx->rfds); -    assert(rc == 0); -    rc = pipe(ctx->wfds); -    assert(rc == 0); -    /* rfd is never readable, since we didn't write anything to the pipe. */ -    ctx->rfd = ctx->rfds[0]; -    ctx->wfd = ctx->wfds[1]; -    /* Make wfd never writeable by filling the pipe. */ -    fill_pipe(ctx->wfd); -} - -void -test_stream_create(struct test_stream *s, size_t maxrd, size_t maxwr) -{ -    test_stream_ctx_create(&s->ctx, maxrd, maxwr); -    s->io.ctx = &s->ctx; -    s->io.vtab = &test_stream_vtab; -    s->io.fd = -1; /* Initialized on demand. */ -} - -void -test_stream_reset(struct test_stream *s, size_t maxrd, size_t maxwr) -{ -    struct test_stream_ctx *ctx = s->io.ctx; -    ctx->avail_rd = maxrd; -    ctx->avail_wr = maxwr; -} - -void -test_stream_destroy(struct test_stream *s) -{ -    iostream_destroy(&s->io); -} diff --git a/test/unit/test_iostream.h b/test/unit/test_iostream.h deleted file mode 100644 index fd8b998aa..000000000 --- a/test/unit/test_iostream.h +++ /dev/null @@ -1,69 +0,0 @@ -/* - * SPDX-License-Identifier: BSD-2-Clause - * - * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file. - */ - -#pragma once - -#include "core/iostream.h" -/** - * A testing stream implementation. Doesn't write anything anywhere, but is - * useful for testing coio behaviour when waiting for stream to become readable - * or writeable. - */ - -#if defined (__cplusplus) -extern "C" { -#endif - -struct test_stream_ctx { -    /** How many bytes can be written before the stream would block. */ -    size_t avail_wr; -    /** How many bytes can be read before the stream would block. */ -    size_t avail_rd; -    /** -     * A file descriptor which is never ready for reads. Points to rfds[0], -     * the reading end of a pipe which is never written to. -     */ -    int rfd; -    /** A pipe used to simulate a fd never ready for reads. */ -    int rfds[2]; -    /** -     * A file descriptor which is never ready for writes. Points to -     * wfds[1], the writing end of a filled up pipe. -     */ -    int wfd; -    /** A pipe to simulate a fd never ready for writes. */ -    int wfds[2]; -}; - -struct test_stream { -    struct iostream io; -    struct test_stream_ctx ctx; -}; - -/** - * Create an instance of a testing iostream. - * The stream will allow reading up to \a maxrd and writing up to \a maxwr - * bytes before blocking. - */ -void -test_stream_create(struct test_stream *s, size_t maxrd, size_t maxwr); - -/** - * Reset the stream. Allow additional \a maxrd and \a maxwr bytes of input / - * output respectively. - */ -void -test_stream_reset(struct test_stream *s, size_t maxrd, size_t maxwr); - -/** - * Destroy the stream. - */ -void -test_stream_destroy(struct test_stream *s); - -#if defined (__cplusplus) -} /* extern "C" */ -#endif =============================== -- Serge Petrenko