Crystal's duplicated stdios are broken

[straight-shoota]

If a standard file descriptor is a TTY, we're duplicating it at the start of the Crystal process in order to set the duplicate to non-blocking without affecting the original file descriptor. See #6518 for a part of the background story.

crystal/src/crystal/system/unix/file_descriptor.cr

Lines 258 to 275 in 6d151b8

	def self.from_stdio(fd)
	# If we have a TTY for stdin/out/err, it is possibly a shared terminal.
	# We need to reopen it to use O_NONBLOCK without causing other programs to break
	# Figure out the terminal TTY name. If ttyname fails we have a non-tty, or something strange.
	# For non-tty we set flush_on_newline to true for reasons described in STDOUT and STDERR docs.
	path = uninitialized UInt8[256]
	ret = LibC.ttyname_r(fd, path, 256)
	return IO::FileDescriptor.new(fd).tap(&.flush_on_newline=(true)) unless ret == 0
	clone_fd = LibC.open(path, LibC::O_RDWR | LibC::O_CLOEXEC)
	return IO::FileDescriptor.new(fd).tap(&.flush_on_newline=(true)) if clone_fd == -1
	# We don't buffer output for TTY devices to see their output right away
	io = IO::FileDescriptor.new(clone_fd)
	io.sync = true
	io
	end

When executing a new process, we must revert this trickery before exec so that the new process image sees the standard file descriptors at the original numbers. The motivation for this is that the process might have altered the duplicated file descriptors which would not have had any effect on the original ones.

crystal/src/crystal/system/unix/process.cr

Lines 472 to 486 in 6d151b8

	private def self.reopen_io(src_io : IO::FileDescriptor, dst_io : IO::FileDescriptor)
	if src_io.closed?
	Crystal::EventLoop.remove(dst_io)
	dst_io.file_descriptor_close
	else
	src_io = to_real_fd(src_io)
	# dst_io.reopen(src_io)
	ret = LibC.dup2(src_io.fd, dst_io.fd)
	raise IO::Error.from_errno("dup2") if ret == -1
	FileDescriptor.set_blocking(dst_io.fd, true)
	dst_io.close_on_exec = false
	end
	end

The major steps are:

• Duping the duplicate back into the original. This is entirely fine.
• Disabling O_CLOEXEC on the original should be safe: it operates on the file descriptor which is exclusive to the child process.
• Disabling O_NONBLOCK has unintended effects: it operates on the underlying file description, which is shared between parent and child process. Setting the child's stdio to blocking changes the parent's blocking status as well.

Demonstration:

# stdin is not a TTY
IO::FileDescriptor.get_blocking(STDIN.fd) # => false
Process.run("true", input: :inherit)
IO::FileDescriptor.get_blocking(STDIN.fd) # => true

The major problem with this code is that it applies to all IOs, even those that were never duped in the first place. It sets all file descriptors to blocking, regardless of their original configuration. 🤦

This may not have drastic practical effects, which probably explains why nobody has noticed before. But it does not behave as expected with potentially breaking semantics.

I'm not sure if this is fixable while preserving behaviour. There might be some means to work around this issue by adding even more complexity on top.
Perhaps the duplicated standard IOs could be made aware of their special function and directly forward changes to the original file descriptors so we wouldn't have to dup them back.

Even better would be if we could remove the need for duplicating the standard streams in the first place. It's a lot of complexity and unexpected behaviour (see #14580 for example).

I'm not even sure why exactly we need this. It has been in the language since practically forever and has changed shape over time a lot. Maybe with the recent enhancements in event loop and detachable schedulers (#15871) we could avoid this?

Related: #15652

---

Add a 👍 reaction to issues you find important.

[ysbaddaden]

Detachable schedulers could simplify, though that could introduce some latency — time for the monitor thread to detect that the thread is blocked.

There's an issue, though: if N fibers are trying to read or write and it blocks, then we might start N threads, and N can be a large number (💣).

[straight-shoota]

I'm trying to wrap my head around understanding this device, but I'm just getting more and more confused.

• Why does this only apply to TTYs? Other file types can be shared with the parent process, too. It seems TTYs are just the most common use case? We change pipes, sockets and character devices to nonblocking. But we only hide that from the parent process if the device is a TTY.
• Why only standard streams? Parent processes can pass additional file descriptors to the child process, which then share the same file description as well. Again, it seems that we're treating the most common use case, but not all.

Concurrently writing to the same file descriptor is usually a bad idea anyway because the output will be jumbled unless you manage to ensure all writes are atomic.
Same for writing, I guess.
So I'm not sure why spawning a number of threads would be the worst part?

Anyway, I suppose we could get to having at most one thread waiting per file descriptor.

[ysbaddaden]

Why does this only apply to TTYs?

Reading #6518 I guess it was the most observable behavior, but since the file descriptions are shared with the parent and child processes, I suppose we should always dup the stdios 🤔

Why only standard streams?

That's the only ones we know about. We might want .shared(fd) or .inherit(fd) on both IO::FileDescriptor and Socket 🤔

[straight-shoota]

I looked into how Go handles blocking file descriptors.
Reading their implementation is quite enlightening. The meat is in newFile: https://github.com/golang/go/blob/099e0027bd7d282c400b1f5930d19a4652a265eb/src/os/file_unix.go#L143
The gist is that there are multiple reasons for a file descriptor being blocking and it might not always be able to change. Go tries to use nonblocking whenever possible, but also strongly maintains the principle that whenever file descriptors are used in some kind of interchange, the expectation is that they must be blocking. So even just querying the file descriptor handle (File.Fd) or pass a file descriptor to a new process, it implicitly changes to blocking mode.
I/O operations on a blocking file descriptor block the entire OS thread. The scheduler can decouple from a tied up thread and continue in a new one. Only a single goroutine can either read or write on a file descriptor. So a maximum of two threads can be tied up on one file descriptor.

I like this. No trickery, no compromises. If a file descriptor is blocking, we must use it in blocking mode.
Performance does not seem to be an issue. At least, I couldn't find any reference to that in the sources. It only mentions that some actions, such as setting a timeout, don't work on blocking file descriptors.

That seems reasonable: We've only been concerned about TTYs as the most common use case in practice. And for stdio TTYs it really shouldn't matter if a maximum of three threads might be tied up in I/O.
And for any use case where performance matters: It should always be possible to alter your program or its inputs to enable nonblocking IO. That requires extra effort, but that's okay. The default behaviour should be safe and expectable.

The main guiding principle must be correctness over performance.
I don't think we can weasel our way out of doing blocking IO without running into some kind of correctness issue.

[ysbaddaden]

Oh, right, with the single-reader and single-writer (#16209) there would be at most 2 blocked threads per fd 👍

About the bug: is there even a point to dup?

We must dup2 before exec to remap the IO args (that can be any custom IO::FileDescriptor) to the actual stdio fds expected by the child process, so input with fd=12 in the parent becomes fd=0 in the child process. This is regardless of the initial dup of the parent stdios.

But dup2 creates a new file descriptor, but shares the same underlying file description under the hood, and O_NONBLOCK is a status flag of the shared underlying file description, not the unique file descriptor, so:

IO::FileDescriptor.get_blocking(STDIN.fd) # => false
IO::FileDescriptor.get_blocking(Crystal::System::ORIGINAL_STDIN.fd) # => false (oops)

Urgh, I was using a non TTY input, that didn't dup. When the input is a TTY it does work (and so I don't understand):

IO::FileDescriptor.get_blocking(STDIN.fd) # => false
IO::FileDescriptor.get_blocking(Crystal::System::ORIGINAL_STDIN.fd) # => true

[ysbaddaden]

Oh, it works because we directly open the TTY device which creates a new file description and can set O_NONBLOCK on it without affecting the parent. We don't dup.

I tried to reproduce, and with or without dup or dup2, setting O_NONBLOCK always leaked the status flag to the parent (as expected).

So, the comment in .from_stdio is wrong: it's not that we're fixing stdio when there's a TTY, it's that we can only fix it when there's a TTY!

[ysbaddaden]

That being said: setting O_NONBLOCK on the stdios might ripple to the shell/terminal process and might break things if stdout is left O_NONBLOCK after the crystal program terminates.

When using a pipe, only the parent-child communication is affected, which might be less visible.

[straight-shoota]

I keep being surprised. Looking at from_stdio suggests that only TTYs should be non-blocking by default.

But look at this: 🧐

$ crystal eval 'p! IO::FileDescriptor.get_blocking(STDOUT.fd)' | cat
IO::FileDescriptor.get_blocking(STDOUT.fd) # => false

Turns out we set even non-TTY file descriptors to non-blocking mode!

This happens when we bail out in from_stdio, we call the public constructor IO::FileDescriptor.new(fd) which happens to "adopt" the file descriptor into the event loop with system_blocking_init:

crystal/src/crystal/system/unix/file_descriptor.cr

Lines 45 to 56 in 3b74bc9

	protected def system_blocking_init(blocking : Bool?)
	if blocking.nil?
	blocking = EventLoop.default_file_blocking? ||
	case system_info.type
	when .pipe?, .socket?, .character_device?
	false
	else
	true
	end
	end
	self.system_blocking = blocking
	end

So we set every standard file descriptor of a pipe or character device¹ to non-blocking.
The only difference for TTYs is that we re-open them as a new file descriptor instead of modifying the original one.

We should probably use the undocumented internal constructor which does not change the blocking mode.

Footnotes

1. I suppose socket shouldn't matter here because you probably can't pass a socket as standard file descriptor? ↩

[straight-shoota]

This actually looks like a failure in API design for IO::FileDescriptor, though.
It offers an easy path to do potentially harmful things.
The default constructor implicitly changes the blocking mode to the expectations of the event loop.
But we have no way to know where the file descriptor comes from and what are the implications for doing that.