af993da435
f_jobstop()/f_rpcstop() .. process_stop() .. process_close_in(proc)
closes the write-stream of a RPC channel. But there might be
a pending RPC notification on the queue, which may get processed just
before the channel is closed.
To handle that case, check the Stream.closed in
channel.c:receive_msgpack().
Before this change, the above scenario could trigger
this assert(!stream->closed) in wstream_write():
0x00007f96e1cd3428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
0x00007f96e1cd502a in __GI_abort () at abort.c:89
0x00007f96e1ccbbd7 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x768f9b "!stream->closed",
file=file@entry=0x768f70 "../src/nvim/event/wstream.c", line=line@entry=77,
function=function@entry=0x768fb0 <__PRETTY_FUNCTION__.13735> "wstream_write") at assert.c:92
0x00007f96e1ccbc82 in __GI___assert_fail (assertion=0x768f9b "!stream->closed", file=0x768f70 "../src/nvim/event/wstream.c", line=77,
function=0x768fb0 <__PRETTY_FUNCTION__.13735> "wstream_write") at assert.c:101
0x00000000004d2c1f in wstream_write (stream=0x7f96e0a35078, buffer=0x7f96e09f9b40) at ../src/nvim/event/wstream.c:77
0x00000000005857b2 in channel_write (channel=0x7f96e0ae5800, buffer=0x7f96e09f9b40) at ../src/nvim/msgpack_rpc/channel.c:551
0x000000000058567d in on_request_event (argv=0x7ffed792efa0) at ../src/nvim/msgpack_rpc/channel.c:523
0x00000000005854c8 in handle_request (channel=0x7f96e0ae5800, request=0x7ffed792f1b8) at ../src/nvim/msgpack_rpc/channel.c:503
0x00000000005850cb in parse_msgpack (channel=0x7f96e0ae5800) at ../src/nvim/msgpack_rpc/channel.c:423
0x0000000000584f90 in receive_msgpack (stream=0x7f96e0a35218, rbuf=0x7f96e0d1d4c0, c=22, data=0x7f96e0ae5800, eof=false)
at ../src/nvim/msgpack_rpc/channel.c:389
0x00000000004d0b20 in read_event (argv=0x7ffed792f4a8) at ../src/nvim/event/rstream.c:190
0x00000000004ce462 in multiqueue_process_events (this=0x7f96e18172d0) at ../src/nvim/event/multiqueue.c:150
0x000000000059b630 in nv_event (cap=0x7ffed792f620) at ../src/nvim/normal.c:7908
0x000000000058be69 in normal_execute (state=0x7ffed792f580, key=-25341) at ../src/nvim/normal.c:1137
0x0000000000652463 in state_enter (s=0x7ffed792f580) at ../src/nvim/state.c:61
0x000000000058a1fe in normal_enter (cmdwin=false, noexmode=false) at ../src/nvim/normal.c:467
0x00000000005500c2 in main (argc=2, argv=0x7ffed792f8d8) at ../src/nvim/main.c:554
Alternative approach suggested by bfredl is to use close_cb of the
process. My unsuccessful attempt is below. (It seems close_cb is queued
too late, which is the similar problem addressed by this commit):
commit 75fc12c6ab15711bdb7b18c6d42ec9d157f5145e
Author: Justin M. Keyes <justinkz@gmail.com>
Date: Fri Aug 18 01:30:41 2017 +0200
rpc: use Stream's close_cb instead of explicit check in receive_msgpack()
diff --git a/src/nvim/event/process.c b/src/nvim/event/process.c
index 8371d3cd482e..e52da23cdc40 100644
--- a/src/nvim/event/process.c
+++ b/src/nvim/event/process.c
@@ -416,6 +416,10 @@ static void on_process_exit(Process *proc)
static void on_process_stream_close(Stream *stream, void *data)
{
Process *proc = data;
+ ILOG("on_process_stream_close");
+ if (proc->stream_close_cb != NULL) {
+ proc->stream_close_cb(stream, proc->stream_close_data);
+ }
decref(proc);
}
diff --git a/src/nvim/event/process.h b/src/nvim/event/process.h
index 5c00e8e7ecd5..34a8d54f6f8c 100644
--- a/src/nvim/event/process.h
+++ b/src/nvim/event/process.h
@@ -26,6 +26,11 @@ struct process {
Stream *in, *out, *err;
process_exit_cb cb;
internal_process_cb internal_exit_cb, internal_close_cb;
+
+ // Called when any of the process streams (in/out/err) closes.
+ stream_close_cb stream_close_cb;
+ void *stream_close_data;
+
bool closed, detach;
MultiQueue *events;
};
@@ -50,6 +55,8 @@ static inline Process process_init(Loop *loop, ProcessType type, void *data)
.closed = false,
.internal_close_cb = NULL,
.internal_exit_cb = NULL,
+ .stream_close_cb = NULL,
+ .stream_close_data = NULL,
.detach = false
};
}
diff --git a/src/nvim/event/stream.c b/src/nvim/event/stream.c
index 7c865bfe1e8c..c8720d1e45d9 100644
--- a/src/nvim/event/stream.c
+++ b/src/nvim/event/stream.c
@@ -95,7 +95,11 @@ void stream_close(Stream *stream, stream_close_cb on_stream_close, void *data)
void stream_close_handle(Stream *stream)
FUNC_ATTR_NONNULL_ALL
{
+ ILOG("stream=%d", stream);
+ // LOG_CALLSTACK();
if (stream->uvstream) {
+ // problem: this schedules on the queue, but channel.c:receive_msgpack may
+ // be processed before close_cb is called by libuv.
uv_close((uv_handle_t *)stream->uvstream, close_cb);
} else {
uv_close((uv_handle_t *)&stream->uv.idle, close_cb);
@@ -105,6 +109,7 @@ void stream_close_handle(Stream *stream)
static void close_cb(uv_handle_t *handle)
{
Stream *stream = handle->data;
+ ILOG(">>>>>>>>>>>>>>>>>>>>>>> stream=%p stream->internal_close_cb=%p", stream, stream->internal_close_cb);
if (stream->buffer) {
rbuffer_free(stream->buffer);
}
diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c
index 782eabe04e4a..dc2b794e366a 100644
--- a/src/nvim/msgpack_rpc/channel.c
+++ b/src/nvim/msgpack_rpc/channel.c
@@ -128,6 +128,8 @@ uint64_t channel_from_process(Process *proc, uint64_t id, char *source)
source);
incref(channel); // process channels are only closed by the exit_cb
channel->data.proc = proc;
+ channel->data.proc->stream_close_cb = close_cb2;
+ channel->data.proc->stream_close_data = channel;
wstream_init(proc->in, 0);
rstream_init(proc->out, 0);
@@ -387,17 +389,6 @@ static void receive_msgpack(Stream *stream, RBuffer *rbuf, size_t c,
goto end;
}
- if ((chan_wstream(channel) != NULL && chan_wstream(channel)->closed)
- || (chan_rstream(channel) != NULL && chan_rstream(channel)->closed)) {
- char buf[256];
- snprintf(buf, sizeof(buf),
- "ch %" PRIu64 ": stream closed unexpectedly. "
- "closing channel",
- channel->id);
- call_set_error(channel, buf, WARN_LOG_LEVEL);
- goto end;
- }
-
size_t count = rbuffer_size(rbuf);
DLOG("ch %" PRIu64 ": parsing %u bytes from msgpack Stream: %p",
channel->id, count, stream);
@@ -571,23 +562,6 @@ static Stream *chan_wstream(Channel *chan)
abort();
}
-/// Returns the Stream that a Channel reads from.
-static Stream *chan_rstream(Channel *chan)
-{
- switch (chan->type) {
- case kChannelTypeSocket:
- return &chan->data.stream;
- case kChannelTypeProc:
- return chan->data.proc->out;
- case kChannelTypeStdio:
- return &chan->data.std.in;
- case kChannelTypeInternal:
- return NULL;
- }
- abort();
-}
-
-
static bool channel_write(Channel *channel, WBuffer *buffer)
{
bool success = false;
@@ -799,6 +773,12 @@ static void close_cb(Stream *stream, void *data)
decref(data);
}
+static void close_cb2(Stream *stream, void *data)
+{
+ ILOG("close_cb2");
+ close_channel(data);
+}
+
/// @param source description of source function, rplugin name, TCP addr, etc
static Channel *register_channel(ChannelType type, uint64_t id,
MultiQueue *events, char *source)
Tests
Tests are run by /cmake/RunTests.cmake file, using busted.
For some failures, .nvimlog (or $NVIM_LOG_FILE) may provide insight.
Directory structure
Directories with tests: /test/benchmark for benchmarks, /test/functional for
functional tests, /test/unit for unit tests. /test/config contains *.in
files (currently a single one) which are transformed into *.lua files using
configure_file CMake command: this is for acessing CMake variables in lua
tests. /test/includes contains include files for use by luajit ffi.cdef
C definitions parser: normally used to make macros not accessible via this
mechanism accessible the other way.
Files /test/*/preload.lua contain modules which will be preloaded by busted,
via --helper option. /test/**/helpers.lua contain various “library”
functions, (intended to be) used by a number of tests and not just a single one.
/test/*/**/*_spec.lua are files containing actual tests. Files that do not end
with a _spec.lua are libraries like /test/**/helpers.lua, except that they
have some common topic.
Tests inside /test/unit and /test/functional are normally divided into
groups by the semantic component they are testing.
Environment variables
Test behaviour is affected by environment variables. Currently supported (Functional, Unit, Benchmarks) (when Defined; when set to 1; when defined, treated as Integer; when defined, treated as String; when defined, treated as Number; !must be defined to function properly):
GDB (F) (D): makes nvim instances to be run under gdbserver. It will be
accessible on localhost:7777: use gdb build/bin/nvim, type target remote :7777 inside.
GDBSERVER_PORT (F) (I): overrides port used for GDB.
VALGRIND (F) (D): makes nvim instances to be run under valgrind. Log files
are named valgrind-%p.log in this case. Note that non-empty valgrind log may
fail tests. Valgrind arguments may be seen in /test/functional/helpers.lua.
May be used in conjunction with GDB.
VALGRIND_LOG (F) (S): overrides valgrind log file name used for VALGRIND.
TEST_SKIP_FRAGILE (F) (D): makes test suite skip some fragile tests.
NVIM_PROG, NVIM_PRG (F) (S): override path to Neovim executable (default to
build/bin/nvim).
CC (U) (S): specifies which C compiler to use to preprocess files. Currently
only compilers with gcc-compatible arguments are supported.
NVIM_TEST_MAIN_CDEFS (U) (1): makes ffi.cdef run in main process. This
raises a possibility of bugs due to conflicts in header definitions, despite the
counters, but greatly speeds up unit tests by not requiring ffi.cdef to do
parsing of big strings with C definitions.
NVIM_TEST_PRINT_I (U) (1): makes cimport print preprocessed, but not yet
filtered through formatc headers. Used to debug formatc. Printing is done
with the line numbers.
NVIM_TEST_PRINT_CDEF (U) (1): makes cimport print final lines which will be
then passed to ffi.cdef. Used to debug errors ffi.cdef happens to throw
sometimes.
NVIM_TEST_PRINT_SYSCALLS (U) (1): makes it print to stderr when syscall
wrappers are called and what they returned. Used to debug code which makes unit
tests be executed in separate processes.
NVIM_TEST_RUN_FAILING_TESTS (U) (1): makes itp run tests which are known to
fail (marked by setting third argument to true).
LOG_DIR (FU) (S!): specifies where to seek for valgrind and ASAN log files.
NVIM_TEST_CORE_* (FU) (S): a set of environment variables which specify where
to search for core files. Are supposed to be defined all at once.
NVIM_TEST_CORE_GLOB_DIRECTORY (FU) (S): directory where core files are
located. May be .. This directory is then recursively searched for core files.
Note: this variable must be defined for any of the following to have any effect.
NVIM_TEST_CORE_GLOB_RE (FU) (S): regular expression which must be matched by
core files. E.g. /core[^/]*$. May be absent, in which case any file is
considered to be matched.
NVIM_TEST_CORE_EXC_RE (FU) (S): regular expression which excludes certain
directories from searching for core files inside. E.g. use ^/%.deps$ to not
search inside /.deps. If absent, nothing is excluded.
NVIM_TEST_CORE_DB_CMD (FU) (S): command to get backtrace out of the debugger.
E.g. gdb -n -batch -ex "thread apply all bt full" "$_NVIM_TEST_APP" -c "$_NVIM_TEST_CORE". Defaults to the example command. This debug command may use
environment variables _NVIM_TEST_APP (path to application which is being
debugged: normally either nvim or luajit) and _NVIM_TEST_CORE (core file to
get backtrace from).
NVIM_TEST_CORE_RANDOM_SKIP (FU) (D): makes check_cores not check cores after
approximately 90% of the tests. Should be used when finding cores is too hard
for some reason. Normally (on OS X or when NVIM_TEST_CORE_GLOB_DIRECTORY is
defined and this variable is not) cores are checked for after each test.
NVIM_TEST_RUN_TESTTEST (U) (1): allows running test/unit/testtest_spec.lua
used to check how testing infrastructure works.
NVIM_TEST_TRACE_LEVEL (U) (N): specifies unit tests tracing level: 0
disables tracing (the fastest, but you get no data if tests crash and there was
no core dump generated), 1 or empty/undefined leaves only C function cals and
returns in the trace (faster then recording everything), 2 records all
function calls, returns and lua source lines exuecuted.