win, tcp: fix to avoid reinserting a pending request

[mpenick]

This fix avoids inserting a duplicate pending request in the case where
WSARecv() returns an error (e.g. when a connection has been terminated by its
peer) when uv_read_start() is called in a read callback.

[mpenick]

Relates to the issue #2687. This fixes the issue I'm experiencing and the tests pass locally in a release build, but I haven't yet gone through every code path that uses UV_HANDLE_READ_PENDING.

[saghul]

Thanks for the patch! Any chance you could add a test?

[mpenick]

Yes, it'll be a couple days because I'm deep in other projects.

[saghul]

No problem, thank you!

[mpenick]

I'm able to reproduce with this example:

#include <stdlib.h>
#include <uv.h>

#define ASSERT(x)                                                              \
  do {                                                                         \
    if (!(x)) {                                                                \
      fprintf(stderr, "'" #x "' failed on line %d\n", __LINE__);               \
      abort();                                                                 \
    }                                                                          \
  } while (0);


static uv_tcp_t server;
static uv_tcp_t connection;

static uv_tcp_t client;
static uv_connect_t connect_req;

static void on_write_close_immediately(uv_write_t *req, int status) {
  ASSERT(0 == status);
  uv_close((uv_handle_t*)req->handle, NULL); /* Close immediately */
  free(req);
}

static void on_write(uv_write_t* req, int status) {
  ASSERT(0 == status);
  free(req);
}

static void do_write(uv_stream_t *stream, uv_write_cb cb) {
  uv_write_t *req = (uv_write_t *)malloc(sizeof(uv_write_t));
  uv_buf_t buf;
  buf.base = "1234578";
  buf.len = 8;
  ASSERT(0 == uv_write(req, stream, &buf, 1, cb));
}

static void on_alloc(uv_handle_t *handle, size_t suggested_size,
                     uv_buf_t *buf) {
  static char slab[65536];
  buf->base = slab;
  buf->len = sizeof(slab);
}

static void on_read2(uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf);

static void on_read1(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) {
  ASSERT(nread >= 0);

  do_write(stream, on_write);

  ASSERT(0 == uv_read_stop(stream));

  ASSERT(0 == uv_read_start(stream, on_alloc, on_read2));
}

static void on_read2(uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf) {
  uv_close((uv_handle_t*)stream, NULL);
  uv_close((uv_handle_t*)&server, NULL);
}

static void on_connection(uv_stream_t *server, int status) {
  ASSERT(0 == status);

  ASSERT(0 == uv_tcp_init(server->loop, &connection));

  ASSERT(0 == uv_accept(server, (uv_stream_t *)&connection));

  ASSERT(0 == uv_read_start((uv_stream_t *)&connection, on_alloc, on_read1));
}

static void on_connect(uv_connect_t *req, int status) {
  ASSERT(0 == status);

  do_write((uv_stream_t*)&client, on_write_close_immediately);
}

int main() {
  uv_loop_t loop;

  ASSERT(0 == uv_loop_init(&loop));

  { /* Server */
    struct sockaddr_in addr;

    ASSERT(0 == uv_ip4_addr("0.0.0.0", 8080, &addr));

    ASSERT(0 == uv_tcp_init(&loop, &server));

    ASSERT(0 == uv_tcp_bind(&server, (struct sockaddr*) & addr, 0));

    ASSERT(0 == uv_listen((uv_stream_t*)&server, 10, on_connection));
  }

  { /* Client */
    struct sockaddr_in addr;

    ASSERT(0 == uv_ip4_addr("0.0.0.0", 8080, &addr));
    ASSERT(0 == uv_tcp_init(&loop, &client));

    ASSERT(0 == uv_tcp_connect(&connect_req, &client,
      (const struct sockaddr*) & addr, on_connect));
  }

  uv_run(&loop, UV_RUN_DEFAULT);
}

Output:

Assertion failed: req != current, file c:\...\req-inl.h, line 99

[mpenick]

Seems to be near (if not) 100% reproducible with that example. I need to figure out how to close the server's listening socket to make this into a test for the success case.

Also, that example can be greatly simplified.

[mpenick]

@saghul ok, I pushed a test. Any feedback is very welcome. It should be noted that I was only able to reproduce the issue server-side. I can try again to make it happen client-side.

[mpenick]

Huh, for some reason I'm have trouble reproducing error with the original code when inside the libuv test executable. I'm working on it.

It's all good now. I'm able to reproduce the original issue with the test when unpatched and it passes after being patched (on both Linux/Windows 10).

[mpenick]

Should I rename this test-tcp-read-stop-start-after-write-to-half-open-connection.c? :)

[saghul]

Left some comments, thanks for adding the test?

[saghul]

Suggested change

	uv_write_t *req = (uv_write_t *)malloc(sizeof(uv_write_t));
	uv_write_t *req = malloc(sizeof(*req));

[saghul]

If this doesn't fit in a single line, please line each argument in its own line

[saghul]

not needed

[saghul]

move forward declarations to the top

[bzoz]

Use the copyright from here: https://github.com/libuv/libuv/blob/v1.x/test/test-ipc-heavy-traffic-deadlock-bug.c

[bzoz]

LGTM.

One nit thought: could you rearrange the functions in the test so that the server and client code are separated? Just so it would be easier to follow the callbacks in the code.

[bnoordhuis]

@saghul I think this is waiting on another review from you? Or maybe dismiss your current one if you don't have time right now?

[saghul]

Thanks for the reminder @bnoordhuis ! I guess all that's missing is removing the change to the GYP (R.I.P) file :-)

[bnoordhuis]

https://ci.nodejs.org/job/libuv-test-commit/1896/ - this PR rebased on top of v1.x with the merge conflict resolved and some whitespace fixes applied.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[bnoordhuis]

Rebase + new CI: https://ci.nodejs.org/job/libuv-test-commit/1926/

[mpenick]

Thanks for merging!