Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirecting standard streams feels more hackish/clumsy than it needs to be #14580

Open
MistressRemilia opened this issue May 9, 2024 · 3 comments

Comments

@MistressRemilia
Copy link
Contributor

This is something that came up as a discussion on my Fediverse account earlier.

Sometimes a program may need to redirect stdin/stdout/stderr programmatically at runtime, but it feels like the process to do this in Crystal is more clumsy than it needs to be. The fact that Crystal will change its own stderr's FD to something other than 2 just sorta complicates matters, leaving a program dealing with two separate stderr FD's.

The use-case where I encountered this was with my Benben program, where a C library I bind will sometimes print to stderr with no way to change this behavior via its API. This then corrupts up my TUI display, which is annoying. So I instead redirect both FD 2 and FD 5 to a file, then keeps the original real stderr in memory to prevent it from being GC'd. Doing so requires some code similar to this:

lib LibC
  # Local binding for dup().  We'll name it this to prevent a possible future
  # conflict.
  fun remidup = "dup"(fd : LibC::Int) : LibC::Int
end

class MyProgram
  class_property! errorOutput : File?
  class_property! errorOutputReal : IO::FileDescriptor?
  class_property origError : LibC::Int = -1
  class_property origErrorReal : LibC::Int = -1

  def redirectStandardFiles : Nil
    filename = "/home/alexa/neat-stderr.log" # Or anything

    # Open a File that we'll redirect stderr into.
    newErr = File.open(filename, "w+b")

    # dup() the original stderr and store it in case we need it
    MyProgram.origError = LibC.remidup(STDERR.fd) # Usually fd 5?

    # Change Crystal's stderr to use our new file.
    STDERR.reopen(newErr)

    # Create a new IO::FileDescriptor for the _real_ stderr's FD.
    #
    # This needs to be adjusted for Crystal versions earlier than 1.6.0 since
    # they don't have "close_on_finalize"
    realStderr = IO::FileDescriptor.new(2, close_on_finalize: false)

    # Store both.
    MyProgram.origErrorReal = realStderr.fd
    MyProgram.errorOutputReal = realStderr # Don't let the GC close it

    # Reopen the real stderr to use our file.
    realStderr.reopen(newErr)

    # Don't GC the new file
    MyProgram.errorOutput = newErr
  end
end

The full original code where I do this in Benben can be found here.

Having a much nicer way to do this in the stdlib, where I don't have to bind dup() and create a new IO::FileDescriptor would be great. Maybe something that accepts a block?

@straight-shoota
Copy link
Member

I believe this can be simplified quite a lot. But still, cloning standard streams is an implementation detail of the Crystal runtime and should be unnoticable to the user (as far as possible).
So IMO STDERR.reopen(x) should ideally affect both, the original file descriptor and the clone.
This of course would require the FileDescriptor instance to know the original file descriptor number.

@straight-shoota
Copy link
Member

straight-shoota commented May 9, 2024

Simplified code to redirect stderr:

redirect = File.open("./stderr", "w+")
redirect.close_on_finalize = false

STDERR.reopen(redirect)
if STDERR.fd != 2
  IO::FileDescriptor.new(2, close_on_finalize: false).reopen(redirect)
end

This should achieve pretty much the same as the example from the OP.
There should be no need to keep a reference alive to anything, as long as all FileDescriptor instance are configured with close_on_finalize = false.

Another advice would be to make this redirect conditional on STDERR being a tty. Or more specifically, being the same TTY as STDOUT. If the error output is already piped into a file, for example, redirecting it somewhere else would be detrimental. Not sure if this is already covered in the original code as it's not relevant for the clumsy redirection.

@MistressRemilia
Copy link
Contributor Author

...cloning standard streams is an implementation detail of the Crystal runtime and should be unnoticable to the user (as far as possible). So IMO STDERR.reopen(x) should ideally affect both, the original file descriptor and the clone. This of course would require the FileDescriptor instance to know the original file descriptor number.

This sounds like a very ideal approach, and what I expected when I first tried implementing this a number of months ago. Finding that STDERR.fd would return 5 was surprising and head scratching, and having to bind dup() to get around it is, imo, the most clumsy part since, as you say, it should be an implementation detail.

Another advice would be to make this redirect conditional on STDERR being a tty. Or more specifically, being the same TTY as STDOUT. If the error output is already piped into a file, for example, redirecting it somewhere else would be detrimental. Not sure if this is already covered in the original code as it's not relevant for the clumsy redirection.

I had never thought about checking for a TTY... I'll add that to my original code, thank you ^_^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants