"Session using SocketStream throws ENOENT" test fails with libuv 1.20.3

[jamessan]

Debian's CI infrastructured discovered that this test started failing when libuv was upgraded to 1.20.3 (from 1.18.0):

[ RUN      ] test/session_spec.lua @ 180: Session using SocketStream throws ENOENT error when socket does not exist
test/session_spec.lua:181: Expected a different error.
Caught:
(no error)
Expected:
(string) 'ENOENT'

While this is clearly a behavior change, I'm not sure if it's a regression in libuv or if we were relying on something that we shouldn't have been.

Any insight would be helpful to get this resolved.

I have limited time for the next ~week. I was going to try bisecting libuv, but between the annoying lua-client build (when you don't want bundled deps) and the fact that libuv is pulled in through lua-luv, I won't have time to do that soon.

[justinmk]

Maybe it changed to EINVAL? (libuv/libuv@239ab6b, JuliaLang/julia#26685 )

Though it's weird that the test claims no error was received.

[jamessan]

That change was Windows specific and isn't in the 1.x branch.

[justinmk]

Yeah, but I saw similar changes which suggested that this was a direction they were pursuing. Though it would be a breaking API change so it makes sense that it's not in 1.x.

[jamessan]

libuv/libuv#1741 (specifically libuv/libuv@8f9ba2a) is the most recent commit that causes the test to fail. The commit was present before (and reverted).

I'll see if I can get any more information on why we're not getting an error back, as a simple C file appears to work fine. It looks like something specific to the lua layers.

[jamessan]

Adding some debug "printf"s, this is the difference in behavior:

working libuv:

SocketStream:open via uv.pipe_connect file(/tmp/nvim.sock)
Session.new
Session:request vim_eval
MsgpackRpcStream:write _session:request
MsgpackRpcStream:write vim_eval1 + 1 + 1
SocketStream:write via uv.write data(vim_eval1 + 1 + 1)
Session:_run _msgpack_rpc_stream:read_start
MsgpackRpcStream:read_start via _stream:read_start
SocketStream:read_start via uv.read_start
Session:_run via uv.run
SocketStream:open err(ENOENT)
SocketStream:write ECANCELED
●SocketStream:close via uv.close

broken libuv:

SocketStream:open via uv.pipe_connect file(/tmp/nvim.sock)
Session.new
Session:request vim_eval
MsgpackRpcStream:write _session:request
MsgpackRpcStream:write vim_eval1 + 1 + 1
SocketStream:write via uv.write data(vim_eval1 + 1 + 1)
Session:_run _msgpack_rpc_stream:read_start
MsgpackRpcStream:read_start via _stream:read_start
SocketStream:read_start via uv.read_start
Session:_run via uv.run
SocketStream:open err(ENOENT)
Session:_run uv.run finished
Session:_run _msgpack_rpc_stream:read_stop
SocketStream:read_stop via uv.read_stop
Session:request err(nil) result(nil)
◼SocketStream:close via uv.close

[jamessan]

Ok, so the problem is that self._stream_error gets set during SocketSession.open but libuv now doesn't call uv_write in this case. Since SocketSession:write is the only other place that references self._stream_error, the error isn't propagated.

We were only getting ENOENT propagated before because SocketStream:write was being canceled (err = ECANCELED), which we then overrode by propagating the open error.

diff --git i/nvim/socket_stream.lua w/nvim/socket_stream.lua
index 37465b8..8dfbd53 100644
--- i/nvim/socket_stream.lua
+++ w/nvim/socket_stream.lua
@@ -16,6 +16,9 @@ function SocketStream.open(file)
 end
 
 function SocketStream:write(data)
+  if self._stream_error then
+    error(self._stream_error)
+  end
   uv.write(self._socket, data, function(err)
     if err then
       error(self._stream_error or err)
@@ -24,6 +27,9 @@ function SocketStream:write(data)
 end
 
 function SocketStream:read_start(cb)
+  if self._stream_error then
+    error(self._stream_error)
+  end
   uv.read_start(self._socket, function(err, chunk)
     if err then
       error(err)
@@ -33,6 +39,9 @@ function SocketStream:read_start(cb)
 end
 
 function SocketStream:read_stop()
+  if self._stream_error then
+    error(self._stream_error)
+  end
   uv.read_stop(self._socket)
 end
 
diff --git i/nvim/tcp_stream.lua w/nvim/tcp_stream.lua
index 4d79cf2..4e590e4 100644
--- i/nvim/tcp_stream.lua
+++ w/nvim/tcp_stream.lua
@@ -16,6 +16,9 @@ function TcpStream.open(host, port)
 end
 
 function TcpStream:write(data)
+  if self._stream_error then
+    error(self._stream_error)
+  end
   uv.write(self._socket, data, function(err)
     if err then
       error(self._stream_error or err)
@@ -24,6 +27,9 @@ function TcpStream:write(data)
 end
 
 function TcpStream:read_start(cb)
+  if self._stream_error then
+    error(self._stream_error)
+  end
   uv.read_start(self._socket, function(err, chunk)
     if err then
       error(err)
@@ -33,6 +39,9 @@ function TcpStream:read_start(cb)
 end
 
 function TcpStream:read_stop()
+  if self._stream_error then
+    error(self._stream_error)
+  end
   uv.read_stop(self._socket)
 end
 

The above diff fixes the issue, and passes the tests with both the old and new libuv.

I also think it cleans up the interface a little bit since existing errors on the stream are directly propagated (before performing a subsequent action) rather than relying on something else to fail.

Ideally we could error() directly from where self._stream_error is set, instead of catching that it's set later. However, that caused the tests to fail due to hung/zombie nvim instances.