luvit/luvit

No more pipes to file descriptors

Opened this issue · 3 comments

According to libuv/libuv#2658, pipes to file descriptors are fraught with problems. This is part of what's causing #1083 and #1023

Luvit currently creates pipes to fd when stdout/stdin come from files:

luvit/deps/pretty-print.lua

Lines 322 to 352 in d841d16

if uv.guess_handle(0) == 'tty' then
stdin = assert(uv.new_tty(0, true))
else
stdin = uv.new_pipe(false)
uv.pipe_open(stdin, 0)
end
if uv.guess_handle(1) == 'tty' then
stdout = assert(uv.new_tty(1, false))
width = uv.tty_get_winsize(stdout)
if width == 0 then width = 80 end
-- auto-detect when 16 color mode should be used
local term = getenv("TERM")
if term and (term == 'xterm' or term:find'-256color$') then
defaultTheme = 256
else
defaultTheme = 16
end
else
stdout = uv.new_pipe(false)
uv.pipe_open(stdout, 1)
width = 80
end
loadColors()
if uv.guess_handle(2) == 'tty' then
stderr = assert(uv.new_tty(2, false))
else
stderr = uv.new_pipe(false)
uv.pipe_open(stderr, 2)
end

This should be avoided in favor of something else. Need to look into how other projects handle this (node, etc).

More related issues:

As far as I can tell, this is where Node sets up stdout/stdin/stderr streams

For writable (stdout/stderr), it uses either tty, pipe, or a synchronous fs write stream. For readable (stdin), it uses either tty, pipe, or an async fs read stream.

So Luvit is just handling the file handle type incorrectly.

This change is a little bit more complicated than it first appears, because Luvit expects stdout/stdin to always be a raw uv_stream_t userdata (uv_tty_t or uv_pipe_t) throughout its deps. So, simply making stdout a fs.WriteStreamSync instead leads to various problems wherever stdout is used (and same with stdin being a fs.ReadStream).

As far as I can tell, we have a few options here:

  1. Do like Node does and always wrap stdout/stdin/stderr in Stream wrappers (Writable/Readable). This would be very non-backwards-compatible (since, right now, uv.write(stdout, ...) is expected to work, but it wouldn't work once everything is converted to a wrapper), but it's probably the best long-term solution.
  2. Only use the Stream wrappers for file handles and then make all usages of stdout/stdin in Luvit work for either one (i.e. convert uv.write(stdout, ...) to stdout:write(...) which would work on both types). This would still not be backwards-compatible for file handle types, but pipe and tty types would be unaffected.
  3. Keep using uv_pipe_t to a file for stdout/stdin file handles, and then try to fix any problems that come from it (like not calling uv.shutdown on them when they are file handles). This would be the most backwards-compatible option but it's definitely not a good long-term strategy. Could maybe be used as a quick band-aid approach to get file stdout/stdin to stop aborting, though.

Maybe it's time to start considering luvit 3.0.