Bypass signal handling in interpreted code

[cyangle]

Would this be an acceptable workaround for issue #12241?

It bypasses signal handling in interpreted code and waits until child process terminates.

I also tried forwarding child process exit code to interpreted code via Unix socket, but it doesn't work because it seems like the interpreter fiber never yields to other fibers unless it enters into debugger repl.

@straight-shoota

[straight-shoota]

My comment in #14016 (comment) still stands: What's the point? Do you have a specific use case which this change enables or is it just to get more specs running?

[cyangle]

This allows running shell commands with the interpreter.

And I think it's worth it because several github issues were created due to not being able to run shell commands. And fixing more spec tests is a side effect of fixing Process.run

As for the issue of handling process signals, I don't think it's used as widely as Process.run

This brings a lot of benefits with minimal efforts, so why not?

@straight-shoota

[straight-shoota]

So do you think this is actually a proper bugfix instead of a workaround?

[cyangle]

This is still a workaround with a much smaller impact than #14016.

[HertzDevil]

Process#wait must not block the current thread whether as a bug or by design, especially now that it is asynchronous on compiled Windows as well. IMO at a minimum the LibC.waitpid needs to happen on a fresh system thread.

[ysbaddaden]

Hum, that workaround ain't so bad (fake it until you can make it 🤫).

It's very inefficient, but if it enables running sub-processes in interpreted mode that would be nice. It currently hangs forever, and I regularly trip on that limitation myself + have a bunch of skip "interpreted mode hangs forever on Process.run" in some test suites (e.g. Minitest).

@HertzDevil you mean something like that? the issue is how to enqueue the suspended fiber, but #send_fiber should do the job:

def wait
  {% if flag?(:interpreted) %}
    fiber = Fiber.current
    exit_code = uninitialized Int32

    Thread.new do
      LibC.waitpid(pid, pointerof(exit_code), 0)
      fiber.current_thread.scheduler.send_fiber(fiber)
    end

    fiber.reschedule
    @channel.send(exit_code)
    @channel.close
  {% end %}
  @channel.receive
end

@cyangle if the reenabled specs are calling into Crystal, they may be calling crystal run and not crystal i 🤔 if so, they don't test what they should be.

[cyangle]

I don't think thread works properly when interpreted. There seems to be something wrong with thread local variables.

result = ""
thread = Thread.new do
  result = "From Thread #{Thread.current}"
end

thread.join

puts "#{Thread.current}: #{result}"

[ysbaddaden]

I have nothing more to say about this PR. It's still just a hack, but not being able to spawn a child process is very limiting. Threads are indeed unsupported in interpreted code, so we can't use 'em.

[straight-shoota]

Clearing the milestone because it's not yet read to merge and #14766 might provide a better solution.

[straight-shoota]

I'm closing this because signal handlers work in the interpreter since #14766, so there's no need for a workaround anymore.
The original issue #12241 is still unresolved, but this change doesn't help much in this regard.