[Question] Why delete operator= for the Streams class?

[aherlihy]

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.

[arun11299]

Thanks!
Can you please post a small example which demonstrates the issue ? I can spin up an ubuntu instance and see what fix can be done for it.

[aherlihy]

This is my attempt at a small example :) apologies if it's a bit longer than a usual repro, but I wanted to make sure the class structure was maintained so you could see the order of operations. Here's the gist: https://gist.github.com/aherlihy/9e01c5674853d20c51fdf99c4edc49bf

It's a very simplified version of the program I'm writing that basically treats CLI processes and DB operators. Ideally I'd just store one copy of POpen in each class, instead of putting it in a vector and taking the address (which seems...wrong)

When I run it I get:

 192 ==40981== Process terminating with default action of signal 11 (SIGSEGV): dumping core
 193 ==40981==  General Protection Fault
 194 ==40981==    at 0x1BBEEF: __gnu_cxx::__exchange_and_add(int volatile*, int) (atomicity.h:49)
 195 ==40981==    by 0x1BBF85: __gnu_cxx::__exchange_and_add_dispatch(int*, int) (atomicity.h:82)
 196 ==40981==    by 0x1C3730: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:152)
 197 ==40981==    by 0x1C0782: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (shared_ptr_base.h:730)
 198 ==40981==    by 0x1BD61F: std::__shared_ptr<_IO_FILE, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:1169)
 199 ==40981==    by 0x1C1A51: std::__shared_ptr<_IO_FILE, (__gnu_cxx::_Lock_policy)2>::reset() (shared_ptr_base.h:1287)
 200 ==40981==    by 0x1BFAC8: subprocess::detail::Communication::communicate_threaded(char const*, unsigned long) (subprocess.hpp:1960)
 201 ==40981==    by 0x1BF5E4: subprocess::detail::Communication::communicate(char const*, unsigned long) (subprocess.hpp:1923)
 202 ==40981==    by 0x1BD986: subprocess::detail::Streams::communicate(char const*, unsigned long) (subprocess.hpp:1144)
 203 ==40981==    by 0x1BDA8B: subprocess::Popen::communicate(char const*, unsigned long) (subprocess.hpp:1301)
 204 ==40981==    by 0x1BDB1E: subprocess::Popen::communicate() (subprocess.hpp:1315)
 205 ==40981==    by 0x1C0439: OperatorSP::finalize() (main.cpp:41)
 206 --40981-- REDIR: 0x491e850 (libc.so.6:free) redirected to 0x483c9d0 (free)

[arun11299]

Thanks for the example. There seems to be some issue with moves and copies and it is more of an issue if you use vector. I have modified the program to use List and Allocate the Popen instances. Also modified the "finalize" method. It was trying to get the output from "previous" instance...

#include <iostream>
#include <utility>
#include "subprocess.hpp"
#include <list>

using namespace std;

using namespace subprocess;

class OperatorSP {
public:
    std::list<std::unique_ptr<Popen>> returnValue;
    OperatorSP* prevOp = nullptr;
    vector<string> args;
    string name;

    OperatorSP(string n, vector<string> a) {
        args = std::move(a);
        name = std::move(n);
    }

    void initialize(OperatorSP* previous) {
        prevOp = previous;
    }

    void run() {
        if (prevOp == nullptr) {
            auto p = new Popen(args, output{PIPE}) ;
            returnValue.emplace_back(p);
        } else if (prevOp) {
            // call predecessor
            prevOp->run();
            Popen* previous_popen = prevOp->returnValue.back().get();

            if (name == "grep") {
                auto p = new Popen({"grep", "template"},input{previous_popen->output()}, output{PIPE});
                returnValue.emplace_back(p);
            } 
            else if (name == "cut") {
                auto p = new Popen({"cut", "-d,", "-f", "1"},input{previous_popen->output()}, output{PIPE});
                returnValue.emplace_back(p);
            }
            else
                throw runtime_error(string("unknown type " + name));
        }
    }
    void finalize() {
        auto res = returnValue.back()->communicate().first;
        cout << "result=" << res.buf.data();
    }
};

/**
 * This is the base example, that works great!
 */
void sequential_test() {
    auto cat = Popen({"cat", "../subprocess.hpp"}, output{PIPE});
    auto grep = Popen({"grep", "template"}, input{cat.output()}, output{PIPE});
    auto cut = Popen({"cut", "-d,", "-f", "1"}, input{grep.output()}, output{PIPE});
    auto res = cut.communicate().first;
    std::cout << res.buf.data() << std::endl;
}

/**
 * This is the example that fails, the point is just that the POpen instances are stored as a class attribute
 * but when 'finalize' is called, it will cause a segfault bc stream_->input() is uninitialized.
 */
void volcano_test() {
    auto catOp = OperatorSP("cat", {"cat", "../subprocess.hpp"});
    auto grepOp = OperatorSP( "grep", {"grep", "template"});
    auto cutOp = OperatorSP("cut", {"cut", "-d,", "-f", "1"});

    catOp.initialize(NULL);
    grepOp.initialize(&catOp);
    cutOp.initialize(&grepOp);

    cutOp.run();
    cutOp.finalize();
}

int main() {
    std::cout << "Starting run" << std::endl;
    volcano_test();
    //sequential_test();
    return 0;
}

Let me now if this works for you.

[aherlihy]

Yes, this is great, thank you so much for taking the time to run the example! Using 'new' fixes the all segfaults completely. Works for me!