0

I have a function that creates a new bridge object and stores it as a boost::shared_ptr:

bool proxy::bridge::acceptor::accept_connections() {
    try {
        session_ = boost::shared_ptr<bridge>(new bridge(io_service_));

        acceptor_.async_accept(session_->downstream_socket(),
            boost::bind(&acceptor::handle_accept,
                this, boost::asio::placeholders::error));
    }
    catch (std::exception& e) {
        std::cerr << e.what() << std::endl;
        return false;
    }

return true;
}

void proxy::bridge::acceptor::handle_accept(const boost::system::error_code& error) {
    if (!error) {
        session_->local_connect();
        if (!accept_connections()) {
        }
    }
    else {
        std::cerr << error.message() << std::endl;
    }
}

In the bridge class is a std::future variable defined in the header and initialized in a bridge class method:

//proxy.h
std::mutex add_data_lock;
std::vector<int> global_resource_protected_by_lock;

class proxy {
    //...
    class bridge {
        //...
        std::future<void> f_read;
    };

    class acceptor {
         //...
    };
};

//proxy.cpp
void proxy::bridge::read(const boost::system::error_code& error, const size_t& bytes_transferred) {
    if (!error) {
        if (f_read.valid()) { f_read.wait(); }
        f_read = std::async(std::launch::async, std::bind(&proxy::bridge::handle_add_data, shared_from_this(), bytes_transferred));
    }
    else {
        close();
    }
}

void proxy::bridge::handle_add_data(const size_t& bytes_transferred) {
    add_data_lock.lock();
    //do work here
    add_data_lock.unlock();
}

void proxy::bridge::close() {
    //this is called when this object is no longer useful (ie. connection closed)

    if (f_read.valid()) { f_read.wait(); }

    //other closing functions...
}

The read() method is called repeatedly - the goal is to call handle_add_data() asynchronously and do work in between cycles of read().

However, the dynamically created bridge object never gets deleted even after the socket is closed (process takes more and more memory).

If I replace the async call and future with a direct function call to handle_add_data(), everything is deallocated properly when close() is called.

If I move the f_read future to outside of the class as a static variable with file scope, then everything is deallocated properly as well, but sometimes I get 'mutex destroyed while busy' errors.

If I don't assign the async call to anything, then it blocks when going out of scope of the function and defeats the purpose of using it.

What should I be doing so the dynamically created bridge objects get deleted properly after close() is called?

Sorry for the confusing code - I condensed it as best as I could to illustrate the problem.

  • 3
    Why `boost::shared_ptr` instead of `std::shared_ptr`? Your use of `std::future` implies that your compiler has c++11 support, so it shouldn't be a problem. – Dan M. Jan 14 '19 at 10:03
  • This is a bit of Frankensteinian code I pieced together and half-wrote, so it wasn't really a thing I considered. For future reference, is std::shared_ptr preferable over boost's implementation? – NoviceEngineer Jan 14 '19 at 15:47
  • 1
    `std` implementations are always preferable over boost if they are available on all your targeted platforms (with very rare exceptions such as regex). – Dan M. Jan 14 '19 at 15:58
  • I see! I'll swap them around and keep that in mind for the future. – NoviceEngineer Jan 14 '19 at 16:03

1 Answers1

2

Your bind is holding onto a shared_ptr longer than it needs to. You can change it to explicitly release the captured pointer when it is done.

f_read = std::async(std::launch::async, [=, self = shared_from_this()]() mutable { self->handle_add_data(bytes_transferred); self = nullptr; });

Prior to C++14, you need to do the initialisation as a local that gets captured

auto self = shared_from_this();
f_read = std::async(std::launch::async, [=]() mutable { self->handle_add_data(bytes_transferred); self = nullptr; });
Caleth
  • 52,200
  • 2
  • 44
  • 75
  • 1
    Wouldn't this need to be a mutable lambda? (Also, some people would argue that `self.reset()` is clearer, I don't feel strongly about it currently) – sehe Jan 14 '19 at 11:57
  • release != reset. Which is it here? – rubenvb Jan 14 '19 at 12:25
  • @rubenvb it's colloquial usage, roughly "relinquishing the (shared) ownership from the pointer object". – Caleth Jan 14 '19 at 12:35
  • 1
    @Caleth calling `reset` on a shared_ptr deletes the pointee. So does [assigning `nullptr`](http://coliru.stacked-crooked.com/a/c53a6bcbe7dd49e9). This is not what `release` does. Your colloquial usage is just plain incorrect. – rubenvb Jan 14 '19 at 13:33
  • @rubenvb only if it is the [last owner](http://coliru.stacked-crooked.com/a/b8126579c0f3ee84), which is the intention here – Caleth Jan 14 '19 at 13:35
  • Sorry, I'm not familiar with this syntax. VS is telling me the '=' behind shared_from_this() should be an identifier. Also there appears to be no '}'? – NoviceEngineer Jan 14 '19 at 14:25
  • @NoviceEngineer this is a C++14 capture with initialiser. Prior to C++14 you can write it out as a named functor class – Caleth Jan 14 '19 at 14:34
  • @Caleth Right you are. Haven't found much need to play with `shared_ptr` yet myself. Sorry about the noise! – rubenvb Jan 14 '19 at 14:35
  • I'm on VS2015 and the C++14 version gives these errors: C3629: '=': a capture default can only appear at the beginning of a lambda capture list C3260: 'mutable': skipping unexpected token(s) before lambda body C3493: 'bytes_transferred' cannot be implicitly captured because no default.. C2672: 'std::async': no matching overloaded function found C2893: Failed to specialize function template.. The older version has these: C3260: 'mutable': skipping unexpected token(s) before.. C2678: binary '=': no operator found which takes a left-hand operand of type 'const boost::shared_ptr.. – NoviceEngineer Jan 14 '19 at 15:01
  • I think it works now! Thanks a lot! I think one of the ')' brackets at the end should be a '}' though. – NoviceEngineer Jan 14 '19 at 15:34