arun11299 / cpp-subprocess Goto Github PK

#23
there was a bug in util::read_all() function where it returned invalid data
template <typename Buffer> static inline int read_all(int fd, Buffer& buf)
and i don't know why the function is templated as it's dealing with a vector<char> and it's always called with a vector<char> correct me if i am wrong but anyways i changed it :D.
and we need an api of getting stdout content from a subprocess like what the test case is doing.

The class Buffer has

  std::vector<char> buf;
  size_t length = 0;

This is a bad interface design because the length field duplicates the buf.size(). At what situations can those differ? Should they differ? It is not documented and unclear.

Furthermore, the class can (and arguably should) be replaced with std::string.

Hi @arun11299,

Is it possible to cut a release/tag?

#include "subprocess.hpp"

using namespace subprocess;

int main() {
  std::vector<std::string> flags = {"ls", "nosuchfile"};
  std::string command = subprocess::util::join(flags);
  auto p = subprocess::Popen(command);
  // while (p.poll() == -1) {
  //   usleep(100 * 1000);
  // }

  p.wait();

  std::cout << "result " << p.retcode() << std::endl;
}

Outputs result: -1

While the below outputs result: 0.

#include "subprocess.hpp"

using namespace subprocess;

int main() {
  std::vector<std::string> flags = {"ls", "nosuchfile"};
  std::string command = subprocess::util::join(flags);
  auto p = subprocess::Popen(command);
  while (p.poll() == -1) {
    usleep(100 * 1000);
  }

  // p.wait();

  std::cout << "result " << p.retcode() << std::endl;
}

Hi,

I just noticed that my program is busy-looping and thus using up a lot of CPU resources while waiting for a subprocess to finish. I'm using the Popen class. The root cause seems to be that waitpid() is used with the WNOHANG option in the util::wait_for_child_exit(int pid) function.

Why is WNOHANG being used? Is there a way to avoid busy-looping while waiting for a subprocess to finish?

The current definition is

  static inline
  std::string join(const std::vector<std::string>& vec,
                   const std::string& sep = " ")
  {
    std::string res;
    for (auto& elem : vec) res.append(elem + sep);
    res.erase(--res.end());
    return res;
  }

It assumes that the separator is exactly 1 character, which doesn't have to be the case because the sep argument is a string.
Specifically this line is buggy:

    res.erase(--res.end());

Hi, thank you for writing such a useful package!

I'm trying to write a program where I store a copy of the Popen as a class attribute so that I can access the results at a later time. Right now when I try to assign it, I get an error that I cannot set the value because the assignment operator is deleted. I can work around it by storing the Popen instance in vector, but then I get a segfault when I try to extract the result later with communicate().first.buf().data() because I think perhaps the streams input/output are stored only by reference so go out of scope. Fun fact that it works on mac but segfaults on ubuntu, but I think it probably should segfault on both :)

Everything works as long as I do all Popen calls within the same function, but unfortunately I need to be able to store the Popen instance somewhere so that I can get the results later with communicate(). Do you know of any possible workaround? I would be happy to hear any advice you have and just modify my local copy of the library if you don't want to make any changes to the source.

Hi,

There are quite many compilation warnings, it'd be nice if they were ironed out (e.g. unsigned/signed comparison, unused parameters).

Thanks.

Or does it provide the option of thread safety?

In my code I'm using an openmp loop where I would like to call sp::Popen.

It looks something like this:

#pragma omp parallel for
for(size_t i = 0; i < v.size(); i++)
{
	auto p = Popen({"subprocess"}, input{PIPE});
	p.communicate(v[i].c_str(), v[i].size() + 1);
}

where v is a std::vector<std::string> but it hangs randomly at the fork() call in subprocess.hpp:1159

Thanks in advance and thanks for the amazing library.

Consider the following code:

#include "subprocess.hpp"

int main() {
    try {
        auto p = subprocess::Popen({"nonexistent-binary"});
        p.wait()

    } catch (const std::runtime_error& e) {
        std::cout << "exception triggered: " << e.what() << std::endl;
    }

    return 0;
}

When running this, it will print an exception message twice:

$ make test
g++ -g -std=c++17 -o subprocess main.cpp -pthread
./subprocess
exception triggered: execve failed : No such file or directory
exception triggered: execve failed : No such file or directory

The exception appears to be thrown in both the parent and the child process in case of execve failure. This seems like a bug as a result of not exiting the forked process.

#include <subprocess.hpp>

using namespace std;
using namespace subprocess;

int main()
{
	vector<Popen> sub_processes;
	for ( int i = 0 ; i < 5 ; i++ )
	{
		sub_processes.emplace_back(Popen({"cat"},input{PIPE}));
		sub_processes[i].send("3 5\n", 5);
	}

	for ( int i = 0 ; i < 5 ; i++ )
	{
		sub_processes[i].wait(); // waits forever
	}
}

In the code above I spawn 5 instances of cat. I call send() to each of them.
In the second loop we call wait but it waits forever. I suspect that cat is still waiting for more input because the channel was not closed.

communicate() does close the channels. But I need to use send() is there a function I am missing that can close the channels?

Thanks in advance.

Is there any support to VC++? If possible in future releases this support could be included and done tests?

I would expect this code:

    using namespace subprocess;
    auto proc = Popen("ls fdklajfdlsajfdkljaf", error{PIPE}, cwd{where}, shell{true});
    auto errors = proc.communicate().first;
    std::cerr << errors.buf.data() << std::endl;
    int exit_code = proc.wait();
    if(exit_code != 0)
    {
        cerr << "the command " << command << " returned error code " << exit_code << endl;
        exit(1);
    }

to set the exit_code variable to 2 and exit. Instead it sets it to 0 and goes on.

My code is set up as follows:

main.h

#include "helper.h"

helper.h

#include "subprocess.hpp"

helper.cpp

#include "helper.h"

Error:

Because of this, I get:

main.cpp.o: In function std::iterator_traits<unsigned char const*>::iterator_category std::__iterator_category<unsigned char const*>(unsigned char const* const&)': subprocess.hpp:152: multiple definition of subprocess::util::quote_argument(std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&, std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >&, bool)'
main.cpp.o:/subprocess.hpp:152: first defined here
helper.cpp.o: In function std::vector<char*, std::allocator<char*> >::reserve(unsigned int)': /subprocess.hpp:152: multiple definition of subprocess::util::quote_argument(std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&, std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >&, bool)'
main.cpp.o:/subprocess.hpp:152: first defined here
collect2: error: ld returned 1 exit status

Solution

This is easily solved by changing void quote_argument(const std::wstring &argument, std::wstring &command_line, bool force) to inline void quote_argument(const std::wstring &argument, std::wstring &command_line, bool force)

* Filenames changed for privacy.

I'm trying to run this command explorer /select,"C:\Windows" using subprocess::call, it should open a file explorer window with the "Windows" folder selected. However, the library always ends up surrounding all arguments with double quotes, and my double quotes are prepended with an extra backslash, causing the file explorer to open the "Documents" folder (no idea why). So, how should I use the library to run this specific command? I tried passing both, a vector and a string to the call command. Maybe something needs to be tweaked so arguments are not always surrounded with double quotes?

MinGW (Min. GNU for Windows) also requires the Windows-specific code, but doesn't #define _MSC_VER. All the Windows-specific fixes are ignored. Using a combined definition would solve this.

Hi,

I'm looking for a way to collect only stderr from a subprocess, can you provide such an example please?

When I compile test_cat and run it, I get the following error:

$ cd test
$ g++ -Wall -lpthread -o test_env test_env.cc
...
$ ./test_env
Test::test_cat_pipe_redirection
END_TEST
Test::test_cat_file_redirection
END_TEST
20480
free(): invalid next size (normal)

In python, it can be directed by passing stdout.

I would like to point out that an identifier like "__SUBPROCESS_HPP_" does eventually not fit to the expected naming convention of the C++ language standard.
Would you like to adjust your selection for unique names?

First - thank you for this effort.
I'm studying on a program that using subprocess，but occurs some problems：
1、“initializer list” cannot convert to “subprocess::environment”
2、Microsoft C++ abnormal： subprocess::OSError
and i work on windows10, visual studio 2017, is there any solutions for these problems，thanks a lot.

On Windows, the call to std::async in execute_process blocks until the process dies naturally, which is problematic if the process does not die and the intended use case is to kill it ourselves.
Could be easily fixed by storing the return value (of type std::future) of the call to std::async as an instance member. This way, the wait on the future would only happen when the instance is destroyed.
This:

  this->cleanup_future_ = std::async(std::launch::async, [this] {
    WaitForSingleObject(this->process_handle_, INFINITE);

    CloseHandle(this->stream_.g_hChildStd_ERR_Wr);
    CloseHandle(this->stream_.g_hChildStd_OUT_Wr);
    CloseHandle(this->stream_.g_hChildStd_IN_Rd);
  });

instead of this:

  std::async(std::launch::async, [this] {
    WaitForSingleObject(this->process_handle_, INFINITE);

    CloseHandle(this->stream_.g_hChildStd_ERR_Wr);
    CloseHandle(this->stream_.g_hChildStd_OUT_Wr);
    CloseHandle(this->stream_.g_hChildStd_IN_Rd);
  });

I see the uses of std::string and setenv after fork. I believe these functions are unsafe, because after fork, the state of memory heap is undefined. I haven't been able to produce crash myself so this issue isn't super critical, but it'd be good to fix them.

For setenv, perhaps we should create a new environ before fork in the parent. For std::string, we can just write a raw string using write(2) and call _exit.

In the following code:

      int read_bytes = util::read_atmost_n(
                                  fdopen(err_rd_pipe, "r"),
                                  err_buf,
                                  SP_MAX_ERR_BUF_SIZ);
      close(err_rd_pipe);

The value returned by fdopen() is not closed. This causes memory leak.
If you call fclose(), it will close err_rd_pipe. So close(err_rd_pipe) is not necessary.

It would not be a stretch to expect this:

    auto proc = Popen(subprocess::util::split(command), error{PIPE}, cwd{where});

to compile.
Unfortunately it gives the following error:

/home/pklos/projects/(...).cpp:70:80: error: no matching function for call to ‘subprocess::Popen::Popen(std::vector<std::__cxx11::basic_string<char> >, subprocess::error, subprocess::cwd)’
     auto proc = Popen(subprocess::util::split(command), error{PIPE}, cwd{where});

I don't see another equivalent of python shlex.split in your library.

I think we missed output(pipe) in the constructor of Popen in the detailed way of example 1 in file readme.md.

Best,
Aimin

https://github.com/arun11299/cpp-subprocess/blob/master/subprocess.hpp#L1875

I wonder if this condition should be negated.

Hello,

Can we have an example to get outputs of the child process in a loop while the child process is still alive. Consider that the child is going to run for a while. Also I need to terminate the child when the parent process is being killed. Is it possible?

I am executing a ps command and multiple lines are return. Why I am just see the first one?
...
auto res = cut.communicate().first;
std::cout << res.buf.data() << std::endl;

Please, how to iterate the res variable?

Thanks

Hi Arun, I'm trying to open a interactive shell script then send command and get response, again and again, but only firstly Popen script can get response by communicate function, after this, used opened Popen handle to 'send' msg, can't get response by communicate. Could you help share how to do this? Appreciate it!

This is a design issue. Basically there is a problem with definitions like this:

struct output
{
  output(int fd): wr_ch_(fd) {}

  output (FILE* fp):output(fileno(fp)) { assert(fp); }

  output(const char* filename) {
    int fd = open(filename, O_APPEND | O_CREAT | O_RDWR, 0640);
    if (fd == -1) throw OSError("File not found: ", errno);
    wr_ch_ = fd;
}
(...)

The problem is that output is not a RAII type that will close the file if not used, but rather a thin wrapper. This is not good for a type that is just a syntactic sugar for parameter passing. The thing is the user will expect to be able to prepare the arguments before the Popen call. If they are prepared but the Popen call is missed, this will leave dangling file handles.
Imagine code like this:

output o;
if(file_given)
    o = output{file_name};
if(condition)
{
    auto p = Popen({ "grep", "f" }, o, input{ PIPE });
    (...)
}

If condition fails, the code will leave a dangling open file.
The solution is to avoid opening files in the Popen argument types, and just store the filenames in them. The actual file opening should be done by Popen itself. Alternatively, there could be a RAII solution here, but it doesn't look as clean (it isn't needed to open the files until the actual Popen call is made).

The issue here is basically that there's no way to specify a timeout. I hacked the code myself with a new exception type, a global constant for timeout, and a few std::chrono calls in wait_for_child_exit(). However, my global constant probably isn't a great way to do this.

Why is this a problem?

Consider that you have a very important process which cannot crash or hang which needs to run a program that potentially behaves badly. If the other program does behave badly (monitored through pipes, i/o, etc), one would probably like to kill and print the stdout/stderr.

Now, presumably kill(9); communicate(); will always return quickly. But suppose it doesn't and your other program really is just deadlocked. Now your main program is going to hang and there's nothing you can do about it.

Hello,

When running multiple subprocess-instances in different threads simultaneously, the program sometimes crashes into "bad file descriptor" when writing into the subprocess pipe.

I will try to add manual thread safety to my program (so that only one thread can use subprocess at a time), just to be sure that this is indeed the cause of the problem. But should the library be thread-safe?

There is a char* == "w" here:

which never seems to succeed. Instead, using strcmp(mode, "w") == 0 works as expected.

I am using function read_all() but process is blocking.
I find this happen here:
int read_bytes = read(fd, buf + rbytes, read_upto - rbytes);

Solution:

      fd_set fds;
      struct timeval timeout;

      timeout.tv_sec = 3; /* timeout in secs */
      timeout.tv_usec = 10000;
      FD_ZERO(&fds);
      FD_SET(fd, &fds);
      if (select(fd + 1, &fds, NULL, NULL, &timeout) > 0) {
        read_bytes = read(fd, buf + rbytes, read_upto - rbytes);
      } else {
        std::cout << "Time out" << std::endl;
      }

It seems that only Popen::poll() sets Popen::retcode_ (which can be retrieved using Popen::retcode()), but Popen::communicate() instead calls Popen::wait() and throws away the result. I tried calling Popen::poll() myself, but it seems that just returns 0 (IIRC waitpid will only work once).

A simple fix seems to be to let communicate() do retcode_ = wait();, which works, but I'm not sure if that's the best approach here (perhaps wait() should just set it?).

I tried reading output from stderr from a process, and it gets all messed up. It seems the length of the buffer returned is correct, but only the first couple of hundred bytes are filled, the rest are zeroes.

Looking at the code, it seems that read_atmost_n does not handle partial reads correctly. Changing this line to the following seems to fix this.

int read_bytes = read(fd, buf + rbytes, read_upto - rbytes)

On a related note, I suspect that if read_all resizes the buffer and causes a realloc, then the buffer variable might become invalid.

This behavior is seen in 126ee29.

Steps to reproduce

Compile and run the following program on a non-Windows system:

#include <iostream>
#include "subprocess.hpp"
using namespace subprocess;

int main(int argc, char **argv) {
    for (int i=0; i<1000; i++) {
        std::cout << i << std::endl;
        auto p = Popen({"grep", "f"}, output{PIPE}, input{PIPE});
        auto msg = "one\ntwo\nthree\nfour\nfive\n";
        p.send(msg, strlen(msg));
        auto res = p.communicate();
    }
}

Expected behavior

The program runs for 1000 iterations and exits.

Actual behavior

The program runs for some iterations and segfaults. The number of iterations changes as ulimit -n is changed.

Likely cause

Running lsof at the end shows that only six file descriptors are open. However, if file descriptor is made accessible via a FILE * via fdopen, it's necessary to close it with fclose. The orphaned file descriptor won't show up in lsof but it will count against the limit.

In the Windows sp::Popen, we have this:

  // Create the child process.
  bSuccess = CreateProcessW(NULL,
                            szCmdline,    // command line
                            NULL,         // process security attributes
                            NULL,         // primary thread security attributes
                            TRUE,         // handles are inherited
                            0,            // creation flags
                            NULL,         // use parent's environment
                            NULL,         // use parent's current directory
                            &siStartInfo, // STARTUPINFOW pointer
                            &piProcInfo); // receives PROCESS_INFORMATION

which "uses parent's environment" i.e. no custom environment variable support.

I'll work on a pull-request to get the functionality on par with *nix.

Some functions are not inline. This will cause multiple definition errors in projects that include your lib.
Examples:

  std::string get_last_error()
  FILE *file_from_handle(HANDLE h, const char *mode)
void configure_pipe(HANDLE* read_handle, HANDLE* write_handle, HANDLE* child_handle)

There is already an issue #34, but it is about a specific function and you have a more general problem.

First - thank you for this effort. I just noticed the repository...

I was wondering what the pros and cons of cpp-subprocess are relative to other C++ libraries regarding process management, piping and such. I remember that Boost has something like this... ah, yes: Boost::Process; and perhaps there still other options.

It would be useful if you could compare and contrast your library with these alternatives, to help users decide what's a better fit for them. Remember that many/most C++ developers don't have experience with Python 2.7's subprocess facilities/library, so we don't automatically know what's offered.

This would fit nicely inside the README, or possibly a Wiki page here on GitHub with a link from the README.

When I kill the subprocess in main process while main process still running, the subprocess becomes a process, and the ps status is Z+.

The three functions there need an inline statement to not produce linker errors.

Cheers!

CPM is a lightweight "package manager" for C++/CMake projects.

It's mostly a convenience around CMake FetchContent.

It would be nice to allow this project to be consumed in such a way, which means that the CMakeLists.txt file needs to expose the header as a CMake consumable project for other projects/IDEs to cleanly consume and reference to.

If it is OK, I will submit a simple PR that resolves this.

Will conan support be beneficial for the project?

I understand this is header-only, but sometimes it's more convenient to just use conan as if it was reguar project.

I'll be happy to open PR if that's beneficial.

You have a lot of static inline functions, such as

  static inline std::vector<std::string>
split(const std::string& str, const std::string& delims=" \t")

The problem is, with static functions, the compiler will create a separate internal copy of the function in every translation unit that has included subprocess.hpp. In large projects this can grow to huge numbers.
Those functions should be just inline, without static. This way they will be visible to the linker, so it will remove the copies.