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

POSIX multi-thread parallel childs invoked with piped std_in often do not receive STDIN close #169

Open
martlaak-gmail opened this issue Aug 18, 2020 · 13 comments

Comments

@martlaak-gmail
Copy link

martlaak-gmail commented Aug 18, 2020

For example this parallel POSIX child invoke with std_in from pipe often hangs on requestProcess->wait(), because child never receives STDIN close:

#include <iostream>
#include <boost/process/child.hpp>
#include <boost/process/io.hpp>
#include <thread>

using namespace std;
namespace bp = boost::process;

void runProcess()
{
  bp::opstream inputStream;
  bp::ipstream outputStream;
  inputStream << "test" << endl;

  unique_ptr<bp::child> requestProcess(new bp::child("\"cat\"", bp::shell,
                                                     bp::std_in < inputStream,
                                                     (bp::std_out & bp::std_err) > outputStream));
  inputStream.pipe().close();
  requestProcess->wait();

  std::string line;
  while (outputStream && std::getline(outputStream, line) && !line.empty())
     cerr << line << std::endl;
}

int main()
{
  std::thread thread1(runProcess);
  std::thread thread2(runProcess);
  thread1.join();
  thread2.join();
}

Note, that:

  • same program does not hang on Windows ("cat" has to be replaced with "more" then)
  • same program does not hang when std_in is read from file (using bp::std_in < "some_file_name.txt")
  • for me it usually does not hang under debugger but hangs 100% of tries while running without debugger
@Boronak
Copy link

Boronak commented Aug 23, 2020

The child processes are inheriting the pipe input filehandle and therefore the pipe can never truly close. Pipes should be created with O_CLOEXEC.

@klemens-morgenstern
Copy link
Collaborator

@Boronak It would be nice if you kept your issue reporting to one issue instead of misusing comments like this.

@klemens-morgenstern
Copy link
Collaborator

Can you try putting the requestProcess->wait(); after the while loop?

@Boronak
Copy link

Boronak commented Aug 24, 2020

@klemens-morgenstern Sorry for being so unclear. The workaround looks like this:

#include <iostream>
#include <boost/process/child.hpp>
#include <boost/process/io.hpp>
#include <thread>

using namespace std;
namespace bp = boost::process;

void runProcess()
{
  int pipefd[2];
  if (pipe2(pipefd, O_CLOEXEC) != 0)
      throw std::system_error{errno, std::generic_category()};
  boost::process::pipe PIPE{pipefd[0], pipefd[1]};

  bp::opstream inputStream{std::move(PIPE)};
  bp::ipstream outputStream;
  inputStream << "test" << endl;

  unique_ptr<bp::child> requestProcess(new bp::child("\"cat\"", bp::shell,
                                                     bp::std_in < inputStream,
                                                     (bp::std_out & bp::std_err) > outputStream));
  inputStream.pipe().close();
  requestProcess->wait();

  std::string line;
  while (outputStream && std::getline(outputStream, line) && !line.empty())
     cerr << line << std::endl;
}

int main()
{
  std::thread thread1(runProcess);
  std::thread thread2(runProcess);
  thread1.join();
  thread2.join();
}

@klemens-morgenstern
Copy link
Collaborator

@Boronak does that work without bp::shell ?

@Boronak
Copy link

Boronak commented Aug 24, 2020

@klemens-morgenstern Yes.

unique_ptr<bp::child> requestProcess(new bp::child("\"cat\"",// bp::shell,

@Boronak
Copy link

Boronak commented Aug 24, 2020

Please note there are some other problems with this code. As @klemens-morgenstern correctly points out the wait is in the wrong place and the whole code is dependant on OS pipe buffers being big enough to take up the slack. This code may easily deadlock in other ways with more data passing through it.

@martlaak-gmail
Copy link
Author

Yes, I confirm that creating inputStream using pipe created with O_CLOEXEC helps to fix my issue. Thanks!

It is still little confusing to me why the O_CLOEXEC flag was not needed when running only one process then (eg. same example without parallel threads works correctly without it)?

@martlaak-gmail
Copy link
Author

martlaak-gmail commented Aug 25, 2020

Can you try putting the requestProcess->wait(); after the while loop?

And BTW, this did not help. It then simply blocks on getline(outputStream, line) (as child ("cat") endlessly runs and waits for STDIN to be closed).

@Boronak
Copy link

Boronak commented Aug 25, 2020

@martlaak-gmail The pipe handles passed to child::child get closed by boost::process, but the cat instance spawned by the other thread justly blindly inherits them and "steals" the handles keeping them open.

@martlaak-gmail
Copy link
Author

@martlaak-gmail The pipe handles passed to child::child get closed by boost::process, but the cat instance spawned by the other thread justly blindly inherits them and "steals" the handles keeping them open.

Yes, but exactly the same happens when only one child is launched. Why it is never problem then?

@Boronak
Copy link

Boronak commented Aug 26, 2020

Single thread:

Thread 1:
    Make pipe 1
    Fork process 1
Process 1:
    Inherit pipe 1
    Dup pipe 1 to STDIN
    Close pipe 1
    Exec -> cat

Multi thread:

Thread 1:
    Make pipe 1
Thread 2:
    Make pipe 2
Thread 1:
   Fork process 1
Process 1:
   Inherit pipe 1
   Inherit pipe 2
   Dup pipe 1 to STDIN
   Close pipe 1
   Exec -> cat
   Pipe 2 is still open...

@SignalWhisperer
Copy link

I might have some insight on this. I was facing the same issue where launching a single child process would have no problem, but launching multiple in a multi-threaded fashion caused none of them to receive data from stdin.

I tried to workaround by implementing the code myself using posix_spawn and found the exact same problem. The issue is a race condition where one thread calls fork and before it can call execve, the second thread calls fork. The problem would not occur on Windows as it would be a single call to CreateProcess (or whatever is used) and would be an atomic-like action.

The workaround is to add a mutex to the child spawning, which ensures the calls to fork are not interleaved. As to whether the mutex should be inside the Process library or inside user code, that's up to the project owners. Personally, I think the different behavior between Windows and POSIX/Linux (MT-safety that is) should at least be warned against, if not made an atomic-like operation.

I don't think this library makes any MT-safety promises anywhere, but I think it should be safe to assume that launching a new process should be MT-safe, although that's my own opinion.

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

No branches or pull requests

4 participants