Make reads non-blocking if async is specified.

[sheredom]

This is a reworking / fixing of #76 - with thanks to @legerch for the initial proposal!

[legerch]

Hi, just saw current progress on this POSIX behaviour, I'm glad to see this getting upstream, thank you for looking into this !

[yutotakano]

Looking forward to this! Bet you can't imagine the excitement I felt when I first came across this library a couple of hours ago, found it almost perfect but lacking one thing..., only to discover that there's an extremely recent pull request for just that feature to make it perfect!!

[sheredom]

Yeah there is some non-deterministic windows failure I've been trying to work out to no avail yet. Will ask some of the interwebz if they know!

[caesay]

I'm not sure if I entirely support this feature/change, it was my impression that subprocess_read_stdout should be used in a while loop in a new thread. IMO it's critical that the stdout/stderr have their own reading threads, so that the pipes never fill up and start blocking the child process.

It's not async in the true sense of the word, but it's async in that you can do your reading incrementally (eg. line by line) outside of your main/render thread, and propagate messages back to the main thread when something important gets read.

The idea of doing everything on one thread and just checking if there is something to be read every once and a while, or waiting for the whole process to finish before reading doesn't seem like a good approach.

Am I missing something here?

[sheredom]

I think its definitely nice from an API perspective that you could just have a single thread and it'd all automagically just work for you. It looks like (at least from my understanding) Windows doesn't make this possible though!

[starseeker]

@caesay It may not be a "good" approach, but it has the great virtue of simplicity - it lets app/lib developers work on their main features without immediately forcing them to also get into the thread management and data migration businesses.

@sheredom If Windows makes it impossible to do, would it be worth revisiting whether it is worth trying the "manage a thread behind the scenes" approach to present the user with the equivalent feature? Or is that not practical?

[caesay]

I reject your concept that juggling multiple responsibilities on a single main thread (eg. simultaneously reading the process output, updating UI, handling user input and more) is more complex than simply spawning a new thread which raises events when a "message" is received. Starting threads is simple and straightforward, and it's (in my opinion) the correct architectural approach too. So having subprocess_read_stdout block on no data is definitely preferrable, or at the very least it should be configurable. Perhaps the authors of the blocking Windows API agreed.

If you have a use-case which requires a non-blocking read, why don't you implement that yourself on top of this library? Just spawn a simple thread with a while loop that reads into a buffer. Add a simple method that, when called, returns what's in the buffer and clears it, or nothing and doesn't block if the buffer is empty. You now have your desired functionality for just a few extra lines of code and +1 thread.

[starseeker]

@caesay The use case I have in mind doesn't involve UI updating or user input and doesn't assume the function in question is in the main thread to begin with - the goal is simply to run a problematic algorithm that may infinite loop and need to be killed as a subprocess, in order to isolate it from the parent process and allow us to bring it down cleanly. The routine is part of a library, so managing UI and I/O events asynchronously is the responsibility of a higher level of the code - I'm just after a way to time out the problematic algorithm. To the best of my knowledge, running it in a subprocess is the only robust way to handle such a case.

I would agree that making the behavior configurable would be a good way to go. As to implementing it myself, that's what I'll do if subprocess.h doesn't provide a convenient way to do what I need "out of the box". Fortunately, I am able to make my code C++ to use the C++11 threading, but I'm lucky. Using threads across both Windows and other platforms in C is a headache - until very recently Visual Studio didn't support C11 threads, so any code needing to support older Windows compilers needs to get platform specific.

[sheredom]

I see both sides here. I don't feel great about spawning a thread on windows so I'm gonna ask around and see if there is a solution I'm unaware of. If not then guarding that behind an additional flag to make it obvious that here are dragons could be ok.

[caesay]

@sheredom If you're determined to make this work, is there a reason you can't use PeekNamedPipe?

If I'm reading the code correctly, you create a named pipe to hold the stdout, so it should be a simple exercise. Call PeekNamedPipe with a null buffer and check if there is pending data to read with lpTotalBytesAvail.

That also lead me to https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-setnamedpipehandlestate which suggests there is a PIPE_NOWAIT mode for ReadFile too, which could be an alternative but it seems to be discouraged.

But if this does work, please make it configurable 🙂 perhaps a subprocess_option_enable_async_no_wait ?

[starseeker]

@sheredom Is there any update on this? Or anything we can do to help?

[sheredom]

Absolutely slammed at work 😄. I'd be happy if you wanted to take what I did and your suggestion and try get the tests passing!