I added missing C bindings and a fallback to use setitimer when POSIX timers aren't available (namely: OpenBSD and Darwin).

I removed the wip label. Please test and report errors and performance issues :)

Now moved to Crystal::System.

Why not avoid the whole timers thing in the fast path by asking libevent if the FD is ready before we try to read/write. For the case of when crystal has exclusive use of the FD, this should always work. If it's racing then yeah you still need timers to stop you blocking when you loose the race.

Also I don't like how all the other syscalls get interrupted, wouldn't the timer be reset if those syscalls are running?

Why not avoid the whole timers thing in the fast path (...) when crystal has exclusive use of the FD

The whole issue is that a process never has exclusive control of standard file descriptors. A fast path would lead to a blocking syscall, but maybe a sub-process will have already read/written it, and we'd hung the crystal process 😢

Or just with threads: we'd have concurrent accesses, and could hung a thread.

I don't like how all the other syscalls get interrupted

Other syscalls won't be running until we get threads, it's kind of a safe guard to retry on EINTR. Since we can't control what users do, like enabling some signals to interrupt syscalls... I believe we should retry on EINTR anyway.

Wouldn't the timer be reset if those syscalls are running?

I don't understand. Even with threads those syscalls don't care about the timers: they're just interrupted when the kernel sends the SIGALRM signal.

With threads, I guess setitimer could lead to the timer being reset in thread A while thread B still needs it. We'll need some emulation of timer_create or something, like a thread with a queue of timers that will (dis)arm setitimer accordingly (or nanosleep and signal(SIGALRM) directly in a loop).

Okay, so this patch doesn't cause the other syscalls to start returning EINTR, but it could if threads were in use? Then yeah I'm fine with adding these checks for future-proofing.

And re: checking with libevent that was in addition to the timers. Given that stdout/stdin are probably more often to not block than block, my suggestion is probably not worth it.

I reanalyzed, and yes: #with_timer only englobs the read(2) and write(2) calls, so the arm -> syscall -> disarm is always synchronous within a single thread. The #wait_writable and #wait_readable that would trigger the event loop (and run whatever code) happens after disarming the timer.

I'm not very fond of this solution, but it's the best I have at hand.

I tried using a thread that would execute the blocking read/write calls, but the thread safe communication / sync kills the overall performance (e.g. 50 concurrent writes was twice slower).

I wonder how many C libraries handle EINTR correctly. This might not interact well with multithreading and C libraries.

I'm not sure that we should really care too much about performance of stdin/out/err right now, so perhaps the thread solution is better.

The timer_create variant ain't fork-safe too, we must recreate the timer_id in forked processes...

I have been wondering how libUV handles this, so I took a cursory glance over the source and found they.. reopen the fds?
https://github.com/libuv/libuv/blob/v1.x/src/unix/tty.c#L114

I'm familiar with this problem, but not this solution.

Oh, that's a good idea, thanks @Timbus !

@Timbus sadly that doesn't work. I tried to dup the file descriptors, but the snippet in #5830 is still broken.

Hmm. Maybe it has something to do with the extra 'master/slave tty' code in there? I didn't even know tty's had such a concept.
The dup code they use is also fairly complicated, too: https://github.com/libuv/libuv/blob/619937c783a05b51fba95cc9a62543deeffe5fa7/src/unix/core.c#L1004

Just seems odd that it wouldn't work, especially with how relevant their code comments were.

I'm still at work so I haven't really looked into it much but I'd guess you also need to set the FD_CLOEXEC flag on the handle as well.. I might try it when I get home. Thanks for the easy test case.

Yeah, so after fully reading the libuv code, it seems the key is to reopen the FD first, not just dup

# The top of main.cr
lib LibC
  fun ttyname_r(fd : Int32, buf : Char*, buffersize : Int32) : Int32
  fun dup3(oldfd : Int32, newfd : Int32, flags : Int32) : Int32
end

#... and at the top of self.main
    buf = UInt8[255]
    [STDIN, STDOUT, STDERR].each do |handle|
      fd = handle.fd
      ret = LibC.ttyname_r(fd, buf, 254)
      if ret == 0
        path = String.new(buf.to_unsafe)
        puts "Got a TTY called #{path}, cloning FD #{fd}"

        # Reopen and tie the FDs together, basically.
        newfd = LibC.open(path, LibC::O_RDWR | LibC::O_CLOEXEC)
        # Don't dup for now.. Crystal doesn't handle it right
        # LibC.dup3(newfd, fd, LibC::O_CLOEXEC)

        # Hacked this in..
        handle.fd = newfd
        handle.blocking = false
      end
    end

    if STDOUT.tty?
      STDOUT.puts "All working I think? BTW stdout is now #{STDOUT.fd}"
      # ^ stdout is now 4
    end

I assume the dup3 is to 'tie' the filehandles together in some way, but it breaks atm when crystal sets blocking IO on its own handles(?), to spawn the child process (see: reopen_io). If you're willing to treat the original handles separately it's all good though:

#in src/process.cr - self.exec_internal, dupe these handles instead
    reopen_io(input, IO::FileDescriptor.new(0), "r")
    reopen_io(output, IO::FileDescriptor.new(1), "w")
    reopen_io(error, IO::FileDescriptor.new(2), "w")

And, now it seems to work:

./test
Got a TTY called /dev/pts/1, cloning FD 0
Got a TTY called /dev/pts/1, cloning FD 1
Got a TTY called /dev/pts/1, cloning FD 2
All working I think? BTW stdout is now 4
call a subprocess
Press CTRL+C
^CCanceled by user

So that's some sort of progress, maybe?

Awesome, good work @Timbus !

Thanks, but since I spent a long time just going round in circles with this one, at this point my brain hurts and I'm not 100% sure it's a clean/full fix, or it just happens to just look like a fix..
Hope it helps pave a way forward, though.

_{My possibly unwanted opinion: I think switching to libuv could make a lot of these small problems go away, since it handles ttys, and the subprocess fork/exec stuff.. Plus it's generally known to be faster}

I have a dumb question: since libuv handle this case, why libevent don't?

libuv has many abstractions for dealing with IO. You use things like uv_open to open a file, uv_spawn to fork a process, etc.. libevent is -- in my limited experience -- more focused on being a convenient wrapper around polling. That means you still need to handle some of the 'extra work'. Compare these two echo servers: https://github.com/eddieh/libevent-echo-server/blob/master/echo-server.c + https://github.com/eddieh/libuv-echo-server/blob/master/echo-server.c

Back on topic: I turned off the filehandle duping on a subprocess exec, performed a dup3 to overwrite the original FDs + set CLOEXEC on them, and stopped setting NONBLOCK on the original file handles, essentially leaving them alone.. And it seems to work for all the scenarios I could test. I could make an initial PR tonight (work don't pay me to hack on crystal -- yet), or maybe @ysbaddaden wants to do it?

Please push your PR.

Not the place to discuss that, but livuv isn't threadsafe, does lots too many things and is deeply tied to nodejs, and thus very opinionated. libevent is threadsafe, merely wraps the underlying poll syscalls, and we only use timers and socket polling, not even DNS (bypasses getaddrinfo or signal handling (don't remember why). We wouldn't use anything from libuv. It's probable that we'll skip it someday for something custom —just like we replaced libpcl with custom assembly for context switch. Having something light will only make it simpler.

This has been obsoleted by #6518.