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

osproc: execProcess and startProcess hang forever (stuck on streams.readLine) #9953

Open
timotheecour opened this issue Dec 13, 2018 · 24 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Dec 13, 2018

/cc @Araq @dom96 this is a serious usability blocker

I'm on OSX

any osproc routine that captures stdout gets stuck forever.

nim c -r -d:case1 $timn_D/tests/nim/all/t0040.nim

before
^CTraceback (most recent call last)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t0040.nim(29) t0040
/Users/timothee/git_clone/nim/timn/tests/nim/all/t0040.nim(23) main
/Users/timothee/git_clone/nim/Nim/lib/pure/osproc.nim(365) execProcess
/Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim(280) readLine
/Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim(162) readChar
/Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim(87) readData
/Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim(413) fsReadData
SIGINT: Interrupted by Ctrl-C.

nim c -r -d:case2 $timn_D/tests/nim/all/t0040.nim

before
^CSIGINT: Interrupted by Ctrl-C.
Traceback (most recent call last)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t0040.nim(29) t0040
/Users/timothee/git_clone/nim/timn/tests/nim/all/t0040.nim(27) main
/Users/timothee/git_clone/nim/timn/tests/nim/all/t0040.nim(16) startProcessWrapper
/Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim(306) readLine
/Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim(75) atEnd
/Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim(408) fsAtEnd
/Users/timothee/git_clone/nim/Nim/lib/system/sysio.nim(233) endOfFile
SIGINT: Interrupted by Ctrl-C.
import osproc
import streams

let cmd = "/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl"

proc startProcessWrapper()=
  var p = startProcess(command = cmd, options={poStdErrToStdOut})
  defer: close(p)
  let sub = outputStream(p)
  while true:
    echo "before"
    let line = readLine(sub)
    echo line
    echo "after"

proc main()=
  when defined(case1):
    echo "before"
    let ret = execProcess(cmd, options={poStdErrToStdOut})
    echo "after"
    echo ret
  when defined(case2):
    startProcessWrapper()

main()

other languages don't have this problem

in D:

import std.process;
import std.stdio;

void main(){
  auto ret = execute("/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl");
  writeln("ret:", ret);
}
rdmd $timn_D/tests/nim/all/t0039.d
ret:Tuple!(int, "status", string, "output")(0, "")

in C++:

#include <cstdio>
#include <iostream>
#include <memory>
#include <stdexcept>
#include <string>
#include <array>

std::string exec(const char* cmd) {
    std::array<char, 128> buffer;
    std::string result;
    std::shared_ptr<FILE> pipe(popen(cmd, "r"), pclose);
    if (!pipe) throw std::runtime_error("popen() failed!");
    while (!feof(pipe.get())) {
        if (fgets(buffer.data(), 128, pipe.get()) != nullptr)
            result += buffer.data();
    }
    return result;
}

int main(){
  auto ret = exec("'/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl'");
  std::cout << "ret:" << ret << std::endl;
}

clang++ -o /tmp/z01 -std=c++14 -g -ferror-limit=7 $timn_D/tests/nim/all/t0041.cpp

/tmp/z01
ret:

note

@timotheecour
Copy link
Member Author

note: since execProcess wraps startProcess, it's only worth looking at -d:case2

here's backtrace with lldb:

nim c --debugger:native -o:/tmp/nim//app -d:case2 /Users/timothee/git_clone//nim//timn//tests/nim/all/t0040.nim
 lldb /tmp/nim//app
(lldb) target create "/tmp/nim//app"
Current executable set to '/tmp/nim//app' (x86_64).
(lldb) r
Process 80120 launched: '/tmp/nim/app' (x86_64)
before
Process 80120 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
    frame #0: 0x00007fff6699ab9e libsystem_kernel.dylib __read_nocancel + 10
libsystem_kernel.dylib`__read_nocancel:
->  0x7fff6699ab9e <+10>: jae    0x7fff6699aba8            ; <+20>
    0x7fff6699aba0 <+12>: movq   %rax, %rdi
    0x7fff6699aba3 <+15>: jmp    0x7fff66999e67            ; cerror_nocancel
    0x7fff6699aba8 <+20>: retq
Target 0: (app) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
  * frame #0: 0x00007fff6699ab9e libsystem_kernel.dylib __read_nocancel + 10
    frame #1: 0x00007fff668ef87b libsystem_c.dylib _sread + 16
    frame #2: 0x00007fff668eeed1 libsystem_c.dylib __srefill1 + 24
    frame #3: 0x00007fff668eeff1 libsystem_c.dylib __srget + 14
    frame #4: 0x00007fff668e8595 libsystem_c.dylib fgetc + 52
    frame #5: 0x000000010001014c app endOfFile_OpxMRqWNQzmofyVQsNQqVA(f=0x00007fff9966e030) + 92 at /Users/timothee/git_clone/nim/Nim/lib/system/sysio.nim:233
    frame #6: 0x000000010001a3ae app fsAtEnd_Aq3rxetdTkDpVSXU7JqLPQ(s=0x000000010014edc8) + 126 at /Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim:408
    frame #7: 0x000000010001ae01 app atEnd_5ebsEcBZaoqxCkfDcpzdgg(s=0x000000010014edc8) + 97 at /Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim:75
    frame #8: 0x000000010001afe8 app readLine_KN3I0OGVr9bF9acs3y6DWpiA(s=0x000000010014edc8) + 120 at /Users/timothee/git_clone/nim/Nim/lib/pure/streams.nim:306
    frame #9: 0x00000001000127fa app startProcessWrapper_caZgCPLQenYzpHlyZdOOig_2 + 410 at /Users/timothee/git_clone/nim/timn/tests/nim/all/t0040.nim:21
    frame #10: 0x0000000100012abd app main_caZgCPLQenYzpHlyZdOOig + 77 at /Users/timothee/git_clone/nim/timn/tests/nim/all/t0040.nim:32
    frame #11: 0x0000000100012cd8 app NimMainModule + 184 at /Users/timothee/git_clone/nim/timn/tests/nim/all/t0040.nim:34
    frame #12: 0x0000000100012c19 app NimMainInner + 9 at /Users/timothee/git_clone/nim/Nim/lib/system.nim:3147
    frame #13: 0x0000000100012d1a app NimMain + 42 at /Users/timothee/git_clone/nim/Nim/lib/system.nim:3155
    frame #14: 0x0000000100012d67 app main(argc=1, args=0x00007ffeefbfc4f0, env=0x00007ffeefbfc500) + 71 at /Users/timothee/git_clone/nim/Nim/lib/system.nim:3162
    frame #15: 0x00007fff6686108d libdyld.dylib start + 1
(lldb)

@alaviss
Copy link
Collaborator

alaviss commented Dec 13, 2018

Do you have any easily reproducible sample that only requires POSIX tools? Not all of us have Sublime Text to test this with.

@Araq
Copy link
Member

Araq commented Dec 13, 2018

These APIs are heavily used. By testament, on OSX too. Nothing hangs. Maybe it's the space in your path?

@timotheecour
Copy link
Member Author

timotheecour commented Dec 13, 2018

i'm not making it up :)
not a space in path issue, same thing after cp "/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl" /tmp/z01 and trying on /tmp/z01

  • some progress though:
    the bug occurs inside startProcess, because:
var p = startProcess(command = cmd, options={poStdErrToStdOut, poEvalCommand})
let sub = outputStream(p)
let f = FileStream(sub).f
c_exec4(f)
void c_exec4(FILE* file) {
// any call to fread or similar will hang here depending on what command I give (some will work, subl with no argument will hang)
}
  • furthermore, the issue is with buffering:
    indeed, everything works If my command is unbuffer /tmp/z01 instead of /tmp/z01

@timotheecour
Copy link
Member Author

@alaviss > Do you have any easily reproducible sample that only requires POSIX tools? Not all of us have Sublime Text to test this with.

not yet but pretty sure it's not specific to sublime text ; it's got to do with stdout buffering and whether application returns no output at all if I were to guess

@timotheecour
Copy link
Member Author

  • other finding:
    subl returns on cmd line (after opening a sublime window)
    but echo yes | /tmp/z01 doesn't return on cmd line, it opens a sublime buffer called subl stdin odEsPn.txt (presumably subl stdin <random file>.txt), writes what you typed to stdin (yes) in that window, and waits for user to close the window, and finally returns on cmd line.

that could explain the bug ; it's still a bug IMO as both D and C++ work in that case.

@alaviss
Copy link
Collaborator

alaviss commented Dec 13, 2018

Here's my speculation: startProcess creates a pipe between the child and the parent, which causes the child's stdin to no longer be connected to the console, which causes sublime to enter the subl stdin buffer.

I think a partial version of poParentStreams that forwards only the stdin may work.

@alaviss
Copy link
Collaborator

alaviss commented Dec 13, 2018

Here's my PoC:

child.nim:

import terminal

if isatty stdin:
  echo "Hi!"
else:
  while true: discard

parent.nim:

import osproc

let ret = execProcess("./child", options={poStdErrToStdOut})

This will hang indefinitely unless poParentStreams is passed to execProcess

@timotheecour
Copy link
Member Author

timotheecour commented Dec 13, 2018

@alaviss thanks for the reply; but passing poParentStreams crashes:

/Users/timothee/git_clone/nim/Nim/lib/pure/osproc.nim(360) execProcess
/Users/timothee/git_clone/nim/Nim/lib/pure/osproc.nim(1251) outputStream
/Users/timothee/git_clone/nim/Nim/lib/system.nim(3930) failedAssertImpl
/Users/timothee/git_clone/nim/Nim/lib/system.nim(3923) raiseAssert
/Users/timothee/git_clone/nim/Nim/lib/system.nim(2969) sysFatal
Error: unhandled exception: /Users/timothee/git_clone/nim/Nim/lib/pure/osproc.nim(1251, 17) poParentStreams notin p.options API usage error: stream access not allowed when you use poParentStreams [AssertionError]
Error: execution of an external program failed: '/Users/timothee/git_clone/nim/timn/tests/nim/all/t0044 '

but I'm guessing you mean the code is osproc would need to implement this, correct?

I think a partial version of poParentStreams that forwards only the stdin may work.

@alaviss
Copy link
Collaborator

alaviss commented Dec 13, 2018 via email

@timotheecour
Copy link
Member Author

In https://dlang.org/phobos/std_process.html
we can pass directly the Files corresponding to stdin, stdout, stderr (which could be either stdout etc, or pointing to an actual file etc)

@trusted Pid spawnProcess(scope const(char[])[] args, File stdin = std.stdio.stdin, File stdout = std.stdio.stdout, File stderr = std.stdio.stderr, const string[string] env = null, Config config = Config.none, scope const char[] workDir = null); 

seems both simpler and more flexible than what we have in osproc:

proc startProcess(command: string; workingDir: string = ""; args: openArray[string] = []; env: StringTableRef = nil; options: set[ProcessOption] = {poStdErrToStdOut})

(i'm taling only about aspect of selecting stdin/stdout/stderr)

timotheecour added a commit to timotheecour/Nim that referenced this issue Dec 13, 2018
@timotheecour
Copy link
Member Author

timotheecour commented Dec 13, 2018

@alaviss thanks a bunch for your help in diagnosing this!, that seems to fix this bug, see #9963 for WIP

@timotheecour timotheecour removed the OX label Dec 13, 2018
@genotrance
Copy link
Contributor

Not sure if #8648 is related.

@zah
Copy link
Member

zah commented Dec 14, 2018

I agree the process API needs to be reworked. I've filed a similar issue here:
#7999

@ghost
Copy link

ghost commented Dec 14, 2018

These APIs are heavily used. By testament, on OSX too. Nothing hangs. Maybe it's the space in your path?

I have this problem too: on nimforum.

@davidgarland
Copy link

Is there any workaround or alternative library for getting a setup like this to work currently? Or are we stuck until the API is reworked?

@timotheecour
Copy link
Member Author

timotheecour commented Jul 24, 2019

I've solved this issue and other problems related to osproc (eg #7999) by doing a full rewrite inspired from D's std/process.d approach (similar to what I had earlier described in #7999 (comment) but a bit more flexible). I don't have time/energy for a PR on this at the moment (notably, only works on linux/osx, didn't test on windows, and not PR ready) but can share a bit more details if anyone is interested.
The API is flexible, allowing one to redirect stdin/stdout/stderr independently (as opposed to all or nothing poParentStreams) to a file, pipe or other descriptor, along with other improvements, and I'm quite happy with the result.
So my advice for whoever wants to take on the task of improving osproc is to look closely at D's std.process

@alaviss
Copy link
Collaborator

alaviss commented Jul 24, 2019

So what is the API that you've arrived at? I'm interested :)

@zah
Copy link
Member

zah commented Jul 24, 2019

@timotheecour, would you post your new code as a github repo? Perhaps the community can help to develop the Windows version and to integrate it with the async libraries. I'd like to have good process management support in our Chronos library for example.

@alehander92
Copy link
Contributor

alehander92 commented Jul 24, 2019

i also wished independent parent streams, so this would be good :O

@zevv
Copy link
Contributor

zevv commented Jul 28, 2019

This one is not related to #8648. In this case the compiler generates too much output blocks because the pipe is full.

@hoijui
Copy link

hoijui commented May 5, 2023

I wrote a tool that makes heavy use of osproc, and this makes it unusable. I am considering switching the whole software to rust because of it.
Please, fix this!

@hoijui
Copy link

hoijui commented May 5, 2023

I am writing a CLI application that uses stdin, stdout and stderr heavily. it also executes a lot of child processes, which also use them. without poParentStreams, the child prcesses hang/freeze my whole software. with poParentStreams, the child processes stdout and stderr output pollute my main softwares stdout and stderr. both are breaking situations.

@hoijui
Copy link

hoijui commented May 5, 2023

finally found a workaround that works for me!

It is essential to close stdin and stderr before trying to read stdout, to prevent hang/freeze:

let process = osproc.startProcess(
  command = "child-cmd",
  args = [],
  env = nil,
  options = {poUsePath})
process.inputStream.close() # NOTE **Essential** - This prevents hanging/freezing when reading stdout below
process.errorStream.close() # NOTE **Essential** - This prevents hanging/freezing when reading stdout below
let (lines, exCode) = process.readLines()
process.close() # This is probably not required

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

9 participants