diff options
| -rw-r--r-- | .valgrind.supp | 52 | ||||
| -rw-r--r-- | cmake/RunTests.cmake | 2 | ||||
| -rw-r--r-- | src/nvim/msgpack_rpc/channel.c | 76 | ||||
| -rw-r--r-- | test/functional/api/server_requests_spec.lua | 56 | ||||
| -rw-r--r-- | test/functional/legacy/options_spec.lua | 16 |
5 files changed, 165 insertions, 37 deletions
diff --git a/.valgrind.supp b/.valgrind.supp index ec4bd50df5..e76616b85d 100644 --- a/.valgrind.supp +++ b/.valgrind.supp @@ -6,7 +6,7 @@ fun:__nss_database_lookup } { - ex_function_1 + interior_ptr1 Memcheck:Leak fun:malloc fun:try_malloc @@ -14,7 +14,7 @@ fun:ex_function } { - ex_function_2 + interior_ptr2 Memcheck:Leak fun:realloc fun:xrealloc @@ -22,7 +22,7 @@ fun:ex_function } { - ex_function_3 + interior_ptr3 Memcheck:Leak fun:malloc fun:strdup @@ -31,6 +31,52 @@ fun:ex_function } { + interior_ptr4 + Memcheck:Leak + fun:malloc + fun:realloc + fun:xrealloc + fun:ga_grow + fun:ex_function +} +{ + interior_ptr5 + Memcheck:Leak + fun:malloc + fun:try_malloc + fun:xmalloc + fun:set_var + fun:set_var_lval +} +{ + interior_ptr6 + Memcheck:Leak + fun:malloc + fun:try_malloc + fun:xmalloc + fun:get_lit_string_tv + fun:eval7 +} +{ + interior_ptr7 + Memcheck:Leak + fun:malloc + fun:strdup + fun:xstrdup + fun:vim_strsave + fun:copy_tv + fun:get_var_tv + fun:eval7 +} +{ + interior_ptr8 + Memcheck:Leak + fun:malloc + fun:try_malloc + fun:xmalloc + fun:dictitem_alloc +} +{ uv_spawn_with_optimizations Memcheck:Leak fun:malloc diff --git a/cmake/RunTests.cmake b/cmake/RunTests.cmake index 9a3e8c296e..0f301a6a10 100644 --- a/cmake/RunTests.cmake +++ b/cmake/RunTests.cmake @@ -1,6 +1,8 @@ get_filename_component(BUSTED_DIR ${BUSTED_PRG} PATH) set(ENV{PATH} "${BUSTED_DIR}:$ENV{PATH}") +set(ENV{VIMRUNTIME} ${WORKING_DIR}/runtime) + if(NVIM_PRG) set(ENV{NVIM_PROG} "${NVIM_PRG}") endif() diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index c2d16d170f..5564bfa1be 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -66,14 +66,23 @@ typedef struct { uint64_t request_id; } RequestEvent; -#define RequestEventFreer(x) -KMEMPOOL_INIT(RequestEventPool, RequestEvent, RequestEventFreer) -kmempool_t(RequestEventPool) *request_event_pool = NULL; +typedef struct { + Channel *channel; + String method; + Array args; +} DelayedNotification; + +#define _noop(x) +KMEMPOOL_INIT(RequestEventPool, RequestEvent, _noop) +KLIST_INIT(DelayedNotification, DelayedNotification, _noop) +static kmempool_t(RequestEventPool) *request_event_pool = NULL; +static klist_t(DelayedNotification) *delayed_notifications = NULL; static uint64_t next_id = 1; static PMap(uint64_t) *channels = NULL; static PMap(cstr_t) *event_strings = NULL; static msgpack_sbuffer out_buffer; +static size_t pending_requests = 0; #ifdef INCLUDE_GENERATED_DECLARATIONS # include "msgpack_rpc/channel.c.generated.h" @@ -83,6 +92,7 @@ static msgpack_sbuffer out_buffer; void channel_init(void) { request_event_pool = kmp_init(RequestEventPool); + delayed_notifications = kl_init(DelayedNotification); channels = pmap_new(uint64_t)(); event_strings = pmap_new(cstr_t)(); msgpack_sbuffer_init(&out_buffer); @@ -173,14 +183,26 @@ bool channel_send_event(uint64_t id, char *name, Array args) { Channel *channel = NULL; - if (id > 0) { - if (!(channel = pmap_get(uint64_t)(channels, id)) || channel->closed) { - api_free_array(args); - return false; - } - send_event(channel, name, args); + if (id && (!(channel = pmap_get(uint64_t)(channels, id)) + || channel->closed)) { + api_free_array(args); + return false; + } + + if (pending_requests) { + DelayedNotification p = { + .channel = channel, + .method = cstr_to_string(name), + .args = args + }; + // Pending request, queue the notification for sending later + *kl_pushp(DelayedNotification, delayed_notifications) = p; } else { - broadcast_event(name, args); + if (channel) { + send_event(channel, name, args); + } else { + broadcast_event(name, args); + } } return true; @@ -206,16 +228,6 @@ Object channel_send_call(uint64_t id, return NIL; } - if (kv_size(channel->call_stack) > 20) { - // 20 stack depth is more than anyone should ever need for RPC calls - api_set_error(err, - Exception, - _("Channel %" PRIu64 " crossed maximum stack depth"), - channel->id); - api_free_array(args); - return NIL; - } - uint64_t request_id = channel->next_request_id++; // Send the msgpack-rpc request send_request(channel, request_id, method_name, args); @@ -223,18 +235,24 @@ Object channel_send_call(uint64_t id, // Push the frame ChannelCallFrame frame = {request_id, false, false, NIL}; kv_push(ChannelCallFrame *, channel->call_stack, &frame); + pending_requests++; event_poll_until(-1, frame.returned); (void)kv_pop(channel->call_stack); + pending_requests--; if (frame.errored) { api_set_error(err, Exception, "%s", frame.result.data.string.data); return NIL; } - if (channel->closed && !kv_size(channel->call_stack)) { + if (!kv_size(channel->call_stack) && channel->closed) { free_channel(channel); } + if (!pending_requests) { + send_delayed_notifications(); + } + return frame.result; } @@ -678,6 +696,7 @@ static void complete_call(msgpack_object *obj, Channel *channel) static void call_set_error(Channel *channel, char *msg) { + ELOG("Msgpack-RPC error: %s", msg); for (size_t i = 0; i < kv_size(channel->call_stack); i++) { ChannelCallFrame *frame = kv_A(channel->call_stack, i); frame->returned = true; @@ -727,6 +746,20 @@ static WBuffer *serialize_response(uint64_t channel_id, return rv; } +static void send_delayed_notifications(void) +{ + DelayedNotification p; + + while (kl_shift(DelayedNotification, delayed_notifications, &p) == 0) { + if (p.channel) { + send_event(p.channel, p.method.data, p.args); + } else { + broadcast_event(p.method.data, p.args); + } + free(p.method.data); + } +} + #if MIN_LOG_LEVEL <= DEBUG_LOG_LEVEL #define REQ "[request] " #define RES "[response] " @@ -764,3 +797,4 @@ static void log_msg_close(FILE *f, msgpack_object msg) fclose(f); } #endif + diff --git a/test/functional/api/server_requests_spec.lua b/test/functional/api/server_requests_spec.lua index b6f56a868c..916bdfd9f0 100644 --- a/test/functional/api/server_requests_spec.lua +++ b/test/functional/api/server_requests_spec.lua @@ -2,8 +2,10 @@ -- `rpcnotify`, to evaluate `rpcrequest` calls we need the client event loop to -- be running. local helpers = require('test.functional.helpers') -local clear, nvim, eval, eq, run, stop = helpers.clear, helpers.nvim, - helpers.eval, helpers.eq, helpers.run, helpers.stop +local clear, nvim, eval = helpers.clear, helpers.nvim, helpers.eval +local eq, run, stop = helpers.eq, helpers.run, helpers.stop +local restart = helpers.restart + describe('server -> client', function() @@ -13,6 +15,7 @@ describe('server -> client', function() clear() cid = nvim('get_api_info')[1] end) + teardown(restart) describe('simple call', function() it('works', function() @@ -65,4 +68,53 @@ describe('server -> client', function() run(on_request, nil, on_setup) end) end) + + describe('requests and notifications interleaved', function() + -- This tests that the following scenario won't happen: + -- + -- server->client [request ] (1) + -- client->server [request ] (2) triggered by (1) + -- server->client [notification] (3) triggered by (2) + -- server->client [response ] (4) response to (2) + -- client->server [request ] (4) triggered by (3) + -- server->client [request ] (5) triggered by (4) + -- client->server [response ] (6) response to (1) + -- + -- If the above scenario ever happens, the client connection will be closed + -- because (6) is returned after request (5) is sent, and nvim + -- only deals with one server->client request at a time. (In other words, + -- the client cannot send a response to a request that is not at the top + -- of nvim's request stack). + -- + -- But above scenario shoudn't happen by the way notifications are dealt in + -- Nvim: they are only sent after there are no pending server->client + -- request(the request stack fully unwinds). So (3) is only sent after the + -- client returns (6). + it('works', function() + local expected = 300 + local notified = 0 + local function on_setup() + eq('notified!', eval('rpcrequest('..cid..', "notify")')) + end + + local function on_request(method, args) + eq('notify', method) + eq(1, eval('rpcnotify('..cid..', "notification")')) + return 'notified!' + end + + local function on_notification(method, args) + eq('notification', method) + if notified == expected then + stop() + return + end + notified = notified + 1 + eq('notified', eval('rpcrequest('..cid..', "notify")')) + end + + run(on_request, on_notification, on_setup) + eq(expected, notified) + end) + end) end) diff --git a/test/functional/legacy/options_spec.lua b/test/functional/legacy/options_spec.lua index fcc39434ff..983d168609 100644 --- a/test/functional/legacy/options_spec.lua +++ b/test/functional/legacy/options_spec.lua @@ -1,20 +1,14 @@ --- Test for ":options". +-- Test if ":options" throws any exception. The options window seems to mess +-- other tests, so restart nvim in the teardown hook local helpers = require('test.functional.helpers') -local clear, feed, insert = helpers.clear, helpers.feed, helpers.insert -local execute, expect = helpers.execute, helpers.expect +local restart, command, clear = helpers.restart, helpers.command, helpers.clear describe('options', function() setup(clear) + teardown(restart) it('is working', function() - insert('result') - - execute("let caught = 'ok'") - execute('try', 'options', 'catch', 'let caught = v:throwpoint . "\n" . v:exception', 'endtry') - execute('buf 1') - execute('$put =caught') - - expect("result\nok") + command('options') end) end) |