sheredom / subprocess.h

🐜 single header process launching solution for C and C++
The Unlicense
1.12k stars 97 forks source link

Deadlock on Windows #16

Closed jlaumon closed 4 years ago

jlaumon commented 4 years ago

Hey,

It seems I get a deadlock if the process I launch has a relatively long output (works at 300 characters, but doesn't work at 450 or more it seems).

What I do is basically:

subprocess_create(...);
subprocess_join(...);
while (fgets(buffer, sizeof(buffer), subprocess_stdout(&process)))
    ...
subprocess_destroy(...);

and it blocks inside join (in WaitForSingleObject()), but only if the output is long (otherwise it works).

If I try to do the join after the fgets, then it blocks inside fgets (even for small outputs).

Any idea? Am I doing something wrong?

sheredom commented 4 years ago

Any chance you could create a small repro - just a simple C/C++ executable that would hang when you run it with subprocess_create. I've tried to simulate the behaviour you are seeing but it all seems to work my end.

Could you also check that the process you are calling is definitely dead when you have joined? (Use task manager or resource monitor should tell you!)

jlaumon commented 4 years ago

Hm I'm having trouble reproducing it as well now. I'm making a small tool that calls perforce on the command line and it seems to work well at home, but it blocked when I tried it at work (unless the output was short, for some reason).

The process was not dead when I checked, it just hanged, doing nothing. If I killed it manually, subprocess returned an error. I guess the problem is either with my setup at work, or with perforce itself 🤷‍♂

My apologies for not having tried to make a small repro first!

sheredom commented 4 years ago

Always happy to help - glad it wasn't my library at fault though! 😄

jlaumon commented 4 years ago

It's me again! I have a repro! (Sorry! 😛)

Here is a zip with a visual studio project that reproduces the problem. The code of the main is below if you prefer. Basically it's the same thing as before, but with a much larger output (not sure what's different with my PC at work).

int main()
{
    subprocess_s s = {};
    const char* command_line[] = { "cmd /C type file.txt", nullptr }; // file.txt is filled with 64KB of text
    subprocess_create(command_line, subprocess_option_inherit_environment, &s);
    subprocess_join(&s, nullptr);

    char buffer[1024];
    while (fgets(buffer, 1024, subprocess_stdout(&s)))
    {
        printf(buffer);
    }

    while (fgets(buffer, 1024, subprocess_stderr(&s)))
    {
        printf(buffer);
    }

    subprocess_destroy(&s);
    return 0;
}

I suspect the problem is that the subprocess blocks during a write because the output pipe is full. Reading from it should unblock it (I think), but currently I wait on the process to finish before reading.

From what I've read online, it seems that even if I wanted to read from the pipe to unblock the write, a deadlock is still possible because there are 2 pipes (stderr and stdout): I could end up blocked trying to read from one pipe while the subprocess is blocked trying to write to the other.

I checked a little bit what other implementations are doing, and it looks like they usually use overlapped IO to avoid blocking (but then you need to create named pipes instead?! Arrgh Windows!) and I'm not sure how _open_osfhandle fits in all this 😬

Edit: just as I was typing this, reproc just removed its use of named pipes (but broke the windows build 🤷‍♂). So if you want to take a look at the code, don't look at master 😁

sheredom commented 4 years ago

Thanks for the repro! I will look into it next week 😄 (currently in the throws of full lurgy with a winter cold!)

sheredom commented 4 years ago

So I've been looking at this and sheesh louise using winsock2.h on windows is just a trainwreck - it'd require me to have a subprocess_init and subprocess_deinit, and it could interfere with any user usage of sockets too - not good.

Gonna look into doing the named pipe approach - while this is totally ugly and makes me wanna 🤢, I think its the least worst option.

sheredom commented 4 years ago

I had a go at fixing this in https://github.com/sheredom/subprocess.h/pull/20 but it doesn't work and honestly have no clue why as of yet.