Pool of curl handles

Question

I am trying to write a simple reusable pool of curl handles. I found this wonderful implementation of the blocking queue.

The pool itself looks like this:

class curl_pool
{
private:
    int        m_size;
    queue<int> m_queue;
    CURL     **m_handles;
public:
    class connection
    {
        friend class curl_pool;
    private:
        curl_pool &m_pool;
        int        m_index;
        connection(curl_pool &pool, int index) : m_pool(pool), m_index(index) {}
    public:
        connection(connection&& that) : m_pool(that.m_pool), m_index(that.m_index) {}
        CURL *handle() const {
            return m_pool.m_handles[m_index];
        }
        void release() {
            m_pool.m_queue.push(m_index);
        }
        virtual ~connection() {
            release();
        }
    };
    connection open() {
        return connection(*this, m_queue.pop());
    }
    curl_pool(const std::string &root_cert, int size) : m_size(size) {
        curl_global_init(CURL_GLOBAL_DEFAULT);
        m_handles = new CURL*[m_size];
        for (int i = 0; i < m_size; i++) {
            CURL *curl = curl_easy_init();
            curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, true);
            curl_easy_setopt(curl, CURLOPT_CAINFO, root_cert.c_str());
            curl_easy_setopt(curl, CURLOPT_TCP_KEEPALIVE, 1L);
            curl_easy_setopt(curl, CURLOPT_TCP_KEEPIDLE, 120L);
            m_handles[i] = curl;
            m_queue.push(i);
        }
    }
    virtual ~curl_pool() {
        for (int i = 0; i < m_size; i++) {
            curl_easy_cleanup(m_handles[i]);
        }
        delete m_handles;
        curl_global_cleanup();
    }
};

This is the intended use:

{
    curl_pool::connection c = pool.open();
    CURL *curl = c.handle();
    curl_easy_setopt(curl, CURLOPT_URL, "https://www.***.com/");
    CURLcode res = curl_easy_perform(curl);
    stream << "Status code: " << res;
}

I am not a C++ person at all, so I am certain there are a lot of things to improve.

Answer 1

Please stop using new and delete.

This is like reading old C code. Modern C++ rarely uses new and practically never uses delete (unless you are building stuff at a very low level). You should be using containers or smart pointers (always question the use of new and if you see a delete you are not using the facilities of the language).

curl_pool(const std::string &root_cert, int size) : m_size(size) {
    m_handles = new CURL*[m_size];
}

virtual ~curl_pool() {
    delete m_handles;
      // Also note this should have been `delete [] m_handles;`
}

This breaks separation of concerns. You are doing memory management and business logic in the same object.

Also because you don't follow the rule of three. Your code is broken and will likely cause crashes.

The best way to fix this is to change m_handles into a vector.

std::vector<CURL*>     m_handles;

Let the vector handle the memory management. You can handle the business logic. PS. You still need to implement the rule of three.

Your connection is dangerious:

    void release() {
        m_pool.m_queue.push(m_index);
    }

What happens if I call release multiple times? I should not be able to break the code from the outside of the interface. Once you have released the handle you should make sure that a subsequent release does nothing and getting a CURL* should return nullptr.

Note I: Calling release() once will break the code. As the object will subsequently be destroyed and call release again.

Note II: Because your object is copyable. Passing it as a parameter will also break the code. As a copy is made and both the original and the new version will eventually be destroyed.

Looks like this should be a movable but not copyable object.