From 5322333b476600d16696f0fb3c69c3c3f14524d4 Mon Sep 17 00:00:00 2001 From: Thiago de Arruda Date: Sun, 2 Nov 2014 15:20:46 -0300 Subject: server: Refactor to ensure server handles are always properly closed If the server fails to start(due to used address for example), the `server_start` function was freeing the handle memory before it was properly removed from libuv event loop queue. Fix that by replacing the `free(server)` call by `uv_close` call, which will take care of freeing the server on the next event loop iteration. Also replace `EMSG` calls by `ELOG`/`WLOG`. --- src/nvim/msgpack_rpc/server.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index 087ba24111..91aca0c37a 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -11,7 +11,7 @@ #include "nvim/ascii.h" #include "nvim/vim.h" #include "nvim/memory.h" -#include "nvim/message.h" +#include "nvim/log.h" #include "nvim/tempfile.h" #include "nvim/map.h" #include "nvim/path.h" @@ -102,12 +102,12 @@ int server_start(const char *endpoint) if (xstrlcpy(addr, endpoint, sizeof(addr)) >= sizeof(addr)) { // TODO(aktau): since this is not what the user wanted, perhaps we // should return an error here - EMSG2("Address was too long, truncated to %s", addr); + WLOG("Address was too long, truncated to %s", addr); } // Check if the server already exists if (pmap_has(cstr_t)(servers, addr)) { - EMSG2("Already listening on %s", addr); + ELOG("Already listening on %s", addr); return 1; } @@ -152,38 +152,30 @@ int server_start(const char *endpoint) } int result; + uv_stream_t *stream = NULL; if (server_type == kServerTypeTcp) { // Listen on tcp address/port uv_tcp_init(uv_default_loop(), &server->socket.tcp.handle); - server->socket.tcp.handle.data = server; result = uv_tcp_bind(&server->socket.tcp.handle, (const struct sockaddr *)&server->socket.tcp.addr, 0); - if (result == 0) { - result = uv_listen((uv_stream_t *)&server->socket.tcp.handle, - MAX_CONNECTIONS, - connection_cb); - if (result) { - uv_close((uv_handle_t *)&server->socket.tcp.handle, free_server); - } - } + stream = (uv_stream_t *)&server->socket.tcp.handle; } else { // Listen on named pipe or unix socket xstrlcpy(server->socket.pipe.addr, addr, sizeof(server->socket.pipe.addr)); uv_pipe_init(uv_default_loop(), &server->socket.pipe.handle, 0); - server->socket.pipe.handle.data = server; result = uv_pipe_bind(&server->socket.pipe.handle, server->socket.pipe.addr); - if (result == 0) { - result = uv_listen((uv_stream_t *)&server->socket.pipe.handle, - MAX_CONNECTIONS, - connection_cb); + stream = (uv_stream_t *)&server->socket.pipe.handle; + } - if (result) { - uv_close((uv_handle_t *)&server->socket.pipe.handle, free_server); - } - } + stream->data = server; + + if (result == 0) { + result = uv_listen((uv_stream_t *)&server->socket.tcp.handle, + MAX_CONNECTIONS, + connection_cb); } assert(result <= 0); // libuv should have returned -errno or zero. @@ -196,13 +188,12 @@ int server_start(const char *endpoint) result = -ENOENT; } } - EMSG2("Failed to start server: %s", uv_strerror(result)); - free(server); + uv_close((uv_handle_t *)stream, free_server); + ELOG("Failed to start server: %s", uv_strerror(result)); return result; } server->type = server_type; - // Add the server to the hash table pmap_put(cstr_t)(servers, addr, server); @@ -221,7 +212,7 @@ void server_stop(char *endpoint) xstrlcpy(addr, endpoint, sizeof(addr)); if ((server = pmap_get(cstr_t)(servers, addr)) == NULL) { - EMSG2("Not listening on %s", addr); + ELOG("Not listening on %s", addr); return; } @@ -255,7 +246,7 @@ static void connection_cb(uv_stream_t *server, int status) result = uv_accept(server, client); if (result) { - EMSG2("Failed to accept connection: %s", uv_strerror(result)); + ELOG("Failed to accept connection: %s", uv_strerror(result)); uv_close((uv_handle_t *)client, free_client); return; } -- cgit From a1dd70b1d0e9ef155c81eeb249f137e763482d10 Mon Sep 17 00:00:00 2001 From: Thiago de Arruda Date: Sun, 2 Nov 2014 16:37:08 -0300 Subject: event: Reintroduce the immediate event queue Commit @264e0d872c("Remove automatic event deferral") removed the immediate event queue because event deferral now had to be explicit. The problem is that while some events don't need to be deferred, they still can result in recursive `event_poll` calls, and recursion is not supported by libuv. Examples of those are msgpack-rpc requests while a server->client request is pending, or signals which can call `mch_exit`(and that will result in `uv_run` calls). To fix the problem, this reintroduces the immediate event queue for events that can potentially result in event loop recursion. The non-deferred events are still processed in `event_poll`, but only after `uv_run` returns. --- src/nvim/msgpack_rpc/channel.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 58e1f403ec..10d180b3b7 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -435,13 +435,7 @@ static void handle_request(Channel *channel, msgpack_object *request) Array args = ARRAY_DICT_INIT; msgpack_rpc_to_array(request->via.array.ptr + 3, &args); - - if (kv_size(channel->call_stack) || !handler.defer) { - call_request_handler(channel, handler, args, request_id); - return; - } - - // Defer calling the request handler. + bool defer = (!kv_size(channel->call_stack) && handler.defer); RequestEvent *event_data = kmp_alloc(RequestEventPool, request_event_pool); event_data->channel = channel; event_data->handler = handler; @@ -450,21 +444,16 @@ static void handle_request(Channel *channel, msgpack_object *request) event_push((Event) { .handler = on_request_event, .data = event_data - }); + }, defer); } static void on_request_event(Event event) { RequestEvent *e = event.data; - call_request_handler(e->channel, e->handler, e->args, e->request_id); - kmp_free(RequestEventPool, request_event_pool, e); -} - -static void call_request_handler(Channel *channel, - MsgpackRpcRequestHandler handler, - Array args, - uint64_t request_id) -{ + Channel *channel = e->channel; + MsgpackRpcRequestHandler handler = e->handler; + Array args = e->args; + uint64_t request_id = e->request_id; Error error = ERROR_INIT; Object result = handler.fn(channel->id, request_id, args, &error); // send the response @@ -477,6 +466,7 @@ static void call_request_handler(Channel *channel, &out_buffer)); // All arguments were freed already, but we still need to free the array free(args.items); + kmp_free(RequestEventPool, request_event_pool, e); } static bool channel_write(Channel *channel, WBuffer *buffer) -- cgit