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

Pipe i/o example in documentation is incorrect #164

Closed
p-mongo opened this issue Feb 10, 2020 · 2 comments
Closed

Pipe i/o example in documentation is incorrect #164

p-mongo opened this issue Feb 10, 2020 · 2 comments

Comments

@p-mongo
Copy link

p-mongo commented Feb 10, 2020

The output to pipe example provided in the readme has several issues:

  1. It does not ever close the reading end of the pipe (r).
  2. It is my understanding that in general on Unix, one end of a pipe must be closed before using the other end. So, for example, the parent process must close the writing end before reading anything from the reading end. This is illustrated in this SO answer and I seem to recall reading about it in literature but I don't have an authoritative reference right now.
  3. It is possible that due to OS buffering the parent will be notified that the child process exited before the reading thread receives EOF. I had this happen on my production system but not on my development system. In this case the background thread raises an exception when it tries to call r.readpartial complaining about the i/o object being closed by another thread. The main thread should join the reading thread after waiting for the launched process before carrying on to close the reading end (per bullet 1). When I added this wait, the entire process hung due to bullet 2.

I suggest using something along the following lines:

    output = ''
    r, w = IO.pipe

    begin
      process.io.stdout = w
      process.start
      w.close

      thread = Thread.new do
        begin
          loop do
            output << r.readpartial(16384)
          end
        rescue EOFError
        end
      end

      process.wait
      thread.join
    ensure
      r.close
    end
sds added a commit that referenced this issue Mar 1, 2020
It was pointed out in #164 that this example could be improved to follow
proper practices.
@sds
Copy link
Collaborator

sds commented Mar 1, 2020

Thanks for the feedback, @p-mongo.

While the examples were meant to be illustrative rather than exhaustive/robust, given others are likely using them with the assumption they will do the "right thing", it's worthwhile to update them. I've done so in 8e809fe, and really appreciate your input!

Wanted to address your specific points as I think they are worth discussing:

  1. It does not explicitly close the read end of the pipe because this happens automatically. If you call r.autoclose? it would return true, meaning it is closed at program exit as object finalizers are executed. With that said, this code sample may be copied verbatim, so it's worth ensuring we call r.close explicitly.

  2. You are slightly incorrect on the justification for this point (perhaps you phrased in a way I'm misunderstanding—if so my apologies), but your recommendation to close the write end of the pipe is correct.

    When utilizing pipes for IPC you must close the file descriptor of the write end of the pipe in the parent process. This ensures that when the (forked) child process finally closes its file descriptor for the write end of the pipe, the parent process will receive an EOF the next time it attempts to read from the read end of the pipe. If the parent process leaves its write end of the pipe open, the EOF won't be sent as there is a still a process with an open file descriptor (the parent in this case).

    You can reproduce this with the following example which will not terminate until you uncomment the w.close line:

    r, w = IO.pipe
    process = ChildProcess.build("sleep" , "1")
    process.io.stdout = w
    process.start
    
    # w.close # Uncomment this to get example to terminate
    
    thread = Thread.new do
      begin
        loop do
          print r.readpartial(16384)
        end
      rescue EOFError # Won't happen unless we call w.close
      end
    end
    
    process.wait
    thread.join

    However, it's important to note that the example will still work if we call w.close after process.wait (as the loop will eventually return EOF in that case), but your suggestion to do before the thread is created is likely better form, so we'll update to do that.

  3. I'm not able to reproduce the behavior you're describing (getting the background thread to error on the IO object being closed by another thread), but I might be misunderstanding the reproduction steps. In any case, agreed that we should call thread.join to ensure the reader exits before the parent terminates, as that is necessary for the example in 2 to work.

Thanks again for the feedback. We've updated the example.

@p-mongo
Copy link
Author

p-mongo commented Mar 1, 2020

👍 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