From deb6fd670479b2e00b99481ce59fdc187408d99d Mon Sep 17 00:00:00 2001 From: Alisue Date: Sun, 6 Aug 2023 23:25:42 +0900 Subject: [PATCH 1/4] feat(msgpack-rpc): show actual request id in error message --- src/nvim/msgpack_rpc/channel.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 1772b0497b..a80424d78b 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -326,8 +326,8 @@ static void parse_msgpack(Channel *channel) char buf[256]; snprintf(buf, sizeof(buf), "ch %" PRIu64 " returned a response with an unknown request " - "id. Ensure the client is properly synchronized", - channel->id); + "id %" PRIu32 ". Ensure the client is properly synchronized", + channel->id, p->request_id); chan_close_with_error(channel, buf, LOGLVL_ERR); } frame->returned = true; From 01fe6b9e6a84338d4752c93a286262d79120f163 Mon Sep 17 00:00:00 2001 From: Alisue Date: Sun, 6 Aug 2023 23:19:29 +0900 Subject: [PATCH 2/4] feat(msgpack_rpc): support out-of-order responses on `msgpack-rpc` Added to support MessagePack-RPC fully compliant clients that do not return responses in request order. Although it is currently not an efficient implementation for full compliance and full compliance cannot be guaranteed, the addition of the new client type `msgpack-rpc` creates a situation where "if the client type is `msgpack-rpc`, then backward compatibility is ignored and full compliance with MessagePack- RPC compliance is justified even if backward compatibility is ignored if the client type is `msgpack-rpc`. --- src/nvim/eval/funcs.c | 2 +- src/nvim/msgpack_rpc/channel.c | 45 +++++++++++++++++++++++++---- src/nvim/msgpack_rpc/channel_defs.h | 11 +++++++ 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c index cf8525e66a..1ed0994222 100644 --- a/src/nvim/eval/funcs.c +++ b/src/nvim/eval/funcs.c @@ -6730,7 +6730,7 @@ static void f_rpcrequest(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) const char *name = NULL; Channel *chan = find_channel(chan_id); if (chan) { - name = rpc_client_name(chan); + name = get_client_info(chan, "name"); } msg_ext_set_kind("rpc_error"); if (name) { diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index a80424d78b..095e392092 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -306,6 +306,17 @@ end: channel_decref(channel); } +static ChannelCallFrame *find_call_frame(RpcState *rpc, uint32_t request_id) +{ + for (size_t i = 0; i < kv_size(rpc->call_stack); i++) { + ChannelCallFrame *frame = kv_Z(rpc->call_stack, i); + if (frame->request_id == request_id) { + return frame; + } + } + return NULL; +} + static void parse_msgpack(Channel *channel) { Unpacker *p = channel->rpc.unpacker; @@ -321,13 +332,15 @@ static void parse_msgpack(Channel *channel) } arena_mem_free(arena_finish(&p->arena)); } else if (p->type == kMessageTypeResponse) { - ChannelCallFrame *frame = kv_last(channel->rpc.call_stack); - if (p->request_id != frame->request_id) { + ChannelCallFrame *frame = channel->rpc.client_type == kClientTypeMsgpackRpc + ? find_call_frame(&channel->rpc, p->request_id) + : kv_last(channel->rpc.call_stack); + if (frame == NULL || p->request_id != frame->request_id) { char buf[256]; snprintf(buf, sizeof(buf), - "ch %" PRIu64 " returned a response with an unknown request " + "ch %" PRIu64 " (type=%" PRIu32 ") returned a response with an unknown request " "id %" PRIu32 ". Ensure the client is properly synchronized", - channel->id, p->request_id); + channel->id, (unsigned)channel->rpc.client_type, p->request_id); chan_close_with_error(channel, buf, LOGLVL_ERR); } frame->returned = true; @@ -691,6 +704,25 @@ void rpc_set_client_info(uint64_t id, Dictionary info) api_free_dictionary(chan->rpc.info); chan->rpc.info = info; + + // Parse "type" on "info" and set "client_type" + const char *type = get_client_info(chan, "type"); + if (type == NULL || strequal(type, "remote")) { + chan->rpc.client_type = kClientTypeRemote; + } else if (strequal(type, "msgpack-rpc")) { + chan->rpc.client_type = kClientTypeMsgpackRpc; + } else if (strequal(type, "ui")) { + chan->rpc.client_type = kClientTypeUi; + } else if (strequal(type, "embedder")) { + chan->rpc.client_type = kClientTypeEmbedder; + } else if (strequal(type, "host")) { + chan->rpc.client_type = kClientTypeHost; + } else if (strequal(type, "plugin")) { + chan->rpc.client_type = kClientTypePlugin; + } else { + chan->rpc.client_type = kClientTypeUnknown; + } + channel_info_changed(chan, false); } @@ -699,14 +731,15 @@ Dictionary rpc_client_info(Channel *chan) return copy_dictionary(chan->rpc.info, NULL); } -const char *rpc_client_name(Channel *chan) +const char *get_client_info(Channel *chan, const char *key) + FUNC_ATTR_NONNULL_ALL { if (!chan->is_rpc) { return NULL; } Dictionary info = chan->rpc.info; for (size_t i = 0; i < info.size; i++) { - if (strequal("name", info.items[i].key.data) + if (strequal(key, info.items[i].key.data) && info.items[i].value.type == kObjectTypeString) { return info.items[i].value.data.string.data; } diff --git a/src/nvim/msgpack_rpc/channel_defs.h b/src/nvim/msgpack_rpc/channel_defs.h index 1b5f0bb298..79aa8c1b13 100644 --- a/src/nvim/msgpack_rpc/channel_defs.h +++ b/src/nvim/msgpack_rpc/channel_defs.h @@ -14,6 +14,16 @@ typedef struct Channel Channel; typedef struct Unpacker Unpacker; +typedef enum { + kClientTypeUnknown = -1, + kClientTypeRemote = 0, + kClientTypeMsgpackRpc = 5, + kClientTypeUi = 1, + kClientTypeEmbedder = 2, + kClientTypeHost = 3, + kClientTypePlugin = 4, +} ClientType; + typedef struct { uint32_t request_id; bool returned, errored; @@ -37,6 +47,7 @@ typedef struct { uint32_t next_request_id; kvec_t(ChannelCallFrame *) call_stack; Dictionary info; + ClientType client_type; } RpcState; #endif // NVIM_MSGPACK_RPC_CHANNEL_DEFS_H From b46e93c5fd2c73b99b618d4954ab8d1c71ad3fb0 Mon Sep 17 00:00:00 2001 From: Alisue Date: Mon, 7 Aug 2023 00:19:51 +0900 Subject: [PATCH 3/4] docs(msgpack_rpc): add "msgpack-rpc" client type --- runtime/doc/api.txt | 6 +++++- runtime/lua/vim/_meta/api.lua | 6 +++++- src/nvim/api/vim.c | 5 ++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/runtime/doc/api.txt b/runtime/doc/api.txt index 793fcd703b..343c63f4b0 100644 --- a/runtime/doc/api.txt +++ b/runtime/doc/api.txt @@ -1336,7 +1336,11 @@ nvim_set_client_info({name}, {version}, {type}, {methods}, {attributes}) • {type} Must be one of the following values. Client libraries should default to "remote" unless overridden by the user. - • "remote" remote client connected to Nvim. + • "remote" remote client connected "Nvim flavored" + MessagePack-RPC (responses must be in reverse order of + requests). |msgpack-rpc| + • "msgpack-rpc" remote client connected to Nvim via + fully MessagePack-RPC compliant protocol. • "ui" gui frontend • "embedder" application using Nvim as a component (for example, IDE/editor implementing a vim mode). diff --git a/runtime/lua/vim/_meta/api.lua b/runtime/lua/vim/_meta/api.lua index c0e4e35e7d..fdf5016b68 100644 --- a/runtime/lua/vim/_meta/api.lua +++ b/runtime/lua/vim/_meta/api.lua @@ -1679,7 +1679,11 @@ function vim.api.nvim_select_popupmenu_item(item, insert, finish, opts) end --- @param type string Must be one of the following values. Client libraries --- should default to "remote" unless overridden by the --- user. ---- • "remote" remote client connected to Nvim. +--- • "remote" remote client connected "Nvim flavored" +--- MessagePack-RPC (responses must be in reverse order of +--- requests). `msgpack-rpc` +--- • "msgpack-rpc" remote client connected to Nvim via +--- fully MessagePack-RPC compliant protocol. --- • "ui" gui frontend --- • "embedder" application using Nvim as a component (for --- example, IDE/editor implementing a vim mode). diff --git a/src/nvim/api/vim.c b/src/nvim/api/vim.c index 4179ae40b8..16e6ab3fe0 100644 --- a/src/nvim/api/vim.c +++ b/src/nvim/api/vim.c @@ -1497,7 +1497,10 @@ Array nvim_get_api_info(uint64_t channel_id, Arena *arena) /// - "commit" hash or similar identifier of commit /// @param type Must be one of the following values. Client libraries should /// default to "remote" unless overridden by the user. -/// - "remote" remote client connected to Nvim. +/// - "remote" remote client connected "Nvim flavored" MessagePack-RPC (responses +/// must be in reverse order of requests). |msgpack-rpc| +/// - "msgpack-rpc" remote client connected to Nvim via fully MessagePack-RPC +/// compliant protocol. /// - "ui" gui frontend /// - "embedder" application using Nvim as a component (for example, /// IDE/editor implementing a vim mode). From b641fc38749a2a52e40fa7eca6c7c41b1d9b031c Mon Sep 17 00:00:00 2001 From: Alisue Date: Mon, 7 Aug 2023 04:10:32 +0900 Subject: [PATCH 4/4] docs(megpack_rpc): add news entry for msgpack-rpc client type --- runtime/doc/news.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/runtime/doc/news.txt b/runtime/doc/news.txt index 637a33b555..56bdd07171 100644 --- a/runtime/doc/news.txt +++ b/runtime/doc/news.txt @@ -152,6 +152,9 @@ The following new APIs and features were added. • Functions that take a severity as an optional parameter (e.g. |vim.diagnostic.get()|) now also accept a list of severities |vim.diagnostic.severity| +• New RPC client type `msgpack-rpc` is added for `nvim_set_client_info` to + support fully MessagePack-RPC compliant clients. + ============================================================================== CHANGED FEATURES *news-changed*