C++ Wrapper for cURL: Multithreading and serializing asynchronous ops

Question

I did this library to help me from one of my projects and decided to publish it on GitHub hoping people might find it useful, convenient and easy to use in their projects too.

It's a header-only library. The library design is inspired by some popular Java code conventions, like naming of classes and the popular usage of chaining call of methods.

As of now, there are only two HTTP methods I made from CppUrl class, which are the GET and POST with the two corresponding "fetch" functions to execute it either on main thread, or on another thread namely: execute() (synchronous) and async() (asynchronous).

I haven't really tested this on some complicated scenarios so if I may ask, what could probably go wrong here? I could smell some data corruption or some deadlocks when this has been used on an intensive and complicated task, but it might just me.

I want the user of this library to type as less as possible of code. So, what do you think of using std::map<std::string, std::string> as a function parameter for passing files on a POST method? Is it suitable (considering the context)? If not, can you suggest another constructive and simpler way?

class CppUrl {
    using ResponseCallback = std::function<void(CURLcode, const std::string&)>;
public:

    class MissingHttpMethodException : std::exception {
    public:
        std::string what() {
            return "No HTTP method used";
        }
    };

    CppUrl() : task(nullptr) {
        if (curlInstanceCount == 0) {
            curl_global_init(CURL_GLOBAL_ALL);
        }
        handle = curl_easy_init();
        if (handle) {
            curlInstanceCount++;
        }
    }

    CppUrl(const CppUrl&) = delete;
    CppUrl& operator= (CppUrl const&);

    ~CppUrl() {
        curlInstanceCount--;
        curl_easy_cleanup(handle);
        cleanTask();
        handle = nullptr;
        // <= incase the value fluctuates
        if (curlInstanceCount <= 0) {
            curl_global_cleanup();
        }
    }

    /**
     * Perform a request with a GET method
     * @param const std::string &url URL to be requested
     * @param callback(CURLcode code, const std::string &response) response callback invoked from other thread
     */
    CppUrl &get(const std::string &url, ResponseCallback callback) {
        waitTask();
        initRequest(url);
        cleanTask();
        task = new std::thread([this, callback] {
            if (handle != nullptr) {
                CURLcode res = curl_easy_perform(handle);
                callback(res, response);
            }
            else {
                // we assume that the request has been cancelled
            }
            this->busy = false;
        });
        return *this;
    }

    /**
    * Perform a request with a POST method
    * @param const std::string &url: URL to be requested
    * @param std::map<std::string, std::string> files: list of pair of file name and file path
    * @param callback(CURLcode code, const std::string &response): response callback invoked from other thread
    */
    CppUrl &post(const std::string &url, std::map<std::string, std::string> files, ResponseCallback callback) {
        waitTask();
        curl_httppost *formpost = nullptr;
        curl_httppost *lastptr = nullptr;
        curl_slist *headerlist = nullptr;

        for (auto &i : files) {
            curl_formadd(&formpost,
                &lastptr,
                CURLFORM_COPYNAME, i.first.c_str(),
                CURLFORM_FILE, i.second.c_str(),
                CURLFORM_END);
        }

        headerlist = curl_slist_append(headerlist, "Content-Type: multipart/form-data");
        initRequest(url);
        curl_easy_setopt(handle, CURLOPT_HTTPPOST, formpost);
        cleanTask();
        task = new std::thread([this, formpost, headerlist, callback] {
            if (handle != nullptr) {
                CURLcode res = curl_easy_perform(handle);
                callback(res, response);
            }
            else {
                // we assume that the request has been cancelled
            }
            curl_formfree(formpost);
            curl_slist_free_all(headerlist);
            this->busy = false;
        });

        return *this;
    }

    void execute() {
        if (task == nullptr) {
            throw MissingHttpMethodException();
        }
        busy = true;
        task->join();
    }

    void async() {
        if (task == nullptr) {
            throw MissingHttpMethodException();
        }
        busy = true;
        task->detach();
    }

    bool isBusy() {
        return busy;
    }

private:
    static size_t writeCallback(char *contents, size_t size, size_t nmemb, std::string *data) {
        data->append(contents, size * nmemb);
        return size * nmemb;
    }

    void initRequest(const std::string &url) {
        response = "";
        curl_easy_reset(handle);
        curl_easy_setopt(handle, CURLOPT_WRITEDATA, &response);
        curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, &CppUrl::writeCallback);
        curl_easy_setopt(handle, CURLOPT_URL, url.c_str());
    }

    void waitTask() {
        while (busy) {}
    }

    void cleanTask() {
        if (task != nullptr) {
            delete task;
            task = nullptr;
        }
    }

private:
    CURL *handle;
    std::thread *task;
    std::string response;
    bool busy;
    static int curlInstanceCount;
};

int CppUrl::curlInstanceCount = 0;

Example usage

CppUrl http1, http2;

http1.get("http://some-webservice.php?ids=123123,123123", 
    [](CURLcode code, const std::string &response) {
    std::cout << "CURLcode: " << code << ", response: " << response << std::endl;
}).async();

std::map<std::string, std::string> files;

files["image1"] = "C:\\Users\\mr5\\Desktop\\image.png";
files["image2"] = "C:\\Users\\mr5\\Desktop\\plant1.jpg";

http2.post("http://some-webservice.php", files,
    [](CURLcode code, const std::string &response) {
    std::cout << "CURLcode: " << code << ", response: " << response << std::endl;
}).execute();

Answer 1

Design Issue

Creating a new thread for every connection is not a good idea. Creating a thread is expensive. Also a single thread can easily handle thousands of connections, so utilizing a single thread for a single connection is very wasteful.

Also the way you are using the threads is all wrong.
This should never happen.

    busy = true;
    task->detach();

All that does is detach the underlying native thread from the thread object so you no longer have any control or way to check the state of the thread.

The std::thread class is the very low level interface (that basically mimics pthreads). You should look at std::asycn, std::future and std::promise. This provides a higher level abstraction to threads that will give you a more modern handle on how to create threads (this is the interface that modern applications should be using). BUT not for this problem.

Bugs

The curl library is a C library and thus knows nothing about C++. It uses functions pointers and assumes they have a C ABI. Thus giving it C++ function pointers is dodgy. You pass a pointer to a static member function. There is nothing in the standard that guarantees this will have the same ABI as a C function (you just happen to be getting lucky with your implementation).

// This is UB
curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, &CppUrl::writeCallback);

You need to write a C function and pass that as the function pointer to C libraries.

extern "C" friend size_t CppUrl_writeCallback(char *contents, size_t size, size_t nmemb, std::string *data)
{
    // STUFF
}
curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, &CppUrl_writeCallback);

Code Rview:

Runtime_Error

I would subclass from std::runtime_error rather than std::exception.

    class MissingHttpMethodException : std::exception {
    public:
        std::string what() {
            return "No HTTP method used";
        }
    };

Also there is no need to write your own what() the default one will return a string you provide to the constructor of the base class (even std::exception has been updated to take a string).

You got the prototype to what wrong.

// This is what it should be.
virtual const char* what() const;

When you override virtual functions add override that will get the compiler to check that you have written the correct override.

You inherit privately from std::exception

This means nobody else sees it as an exception class. You should inherit publicly from it.

    // This is what you want.
    class MissingHttpMethodException : public std::runtime_error {
    public:
        std::string MissingHttpMethodException(std::string const& what)
            : std::runtime_error(what)
        {}
    };

The other question I would ask is. Do you need an exception class? I can't answer that. But there is only really a need for a specific exception class if you can fix the issue. Is there a point in the code that will explicitly catch and fix the exception? This looks more like you are trying to prevent programmer error. Maybe an assert() is a better option (or a better designed interface that can't be used incorrectly).

Don't use one variable for multiple tasks.

    CppUrl() : task(nullptr) {
        if (curlInstanceCount == 0) {
            curl_global_init(CURL_GLOBAL_ALL);
        }
        handle = curl_easy_init();
        if (handle) {
            curlInstanceCount++;
        }
    }

Here you are using curlInstanceCount for two different tasks. You are using it to count the number of currently open handles and to decide if curl has been initialized. This is going to result in multiple calls to curl_global_init() and curl_global_cleanup(). Not what you wanted! I would put global init/cleanup into its own class.

Example:

 int main()
 {
      for(int loop=0;loop < 1000;++loop)
      {
          // Each time though this loop.
          // curl_global_init() will get called each iteration.
          CppUrl   curl;
          curl.get("http://google.com", [](CURLcode code, std::string const& data) {
              std::cout << "Ho: " << code << " " << data << "\n";
          }
      }
 }

You delete the copy constructor but allow assignment?

This interface is not very intuitive.

    CppUrl(const CppUrl&) = delete;
    CppUrl& operator= (CppUrl const&);

Why would you allow one but not the other!

Destructor Waste

    ~CppUrl() {
        // This is a waste of time.
        // Once the destructor ends this object no longer exists.
        // The content of the memory is completely irrelevant.
        handle = nullptr;
    }

Object state check in worker.

    CppUrl &get(const std::string &url, ResponseCallback callback) {
        task = new std::thread([this, callback] {

            // You really want to make this check in the worker thread?
            // Why not make this check before you create the thread.
            // generate an exception.
            //
            // How can an object get in this state?
            // If the constructor does not get you a handle is this object
            // even valid. You should throw in the constructor if your
            // object is not initialized correctly.
            if (handle != nullptr) {
                CURLcode res = curl_easy_perform(handle);
                callback(res, response);
            }
        });
        return *this;
    }

Access to shared state from multiple threads:

    CppUrl &get(const std::string &url, ResponseCallback callback) {
        task = new std::thread([this, callback] {
            // STUFF

            // This member is accessed by the main thread as well
            // as this worker thread. You need to make sure there
            // explicit memory barriors between the accesses otherwise
            // you could end up reading a value cached by the thread on
            // a different processor.
            //
            // Either make this member atomic
            // or put explicit locks around its access.
            this->busy = false;
        });
        return *this;
    }

Expectations:

The execute() and async() interface imply that the get()/post() don't actually do anything until you call execute/async. But in reality the threads have already started. This is bad design (don't confuse your user).

Also they don't work.

The thread doing the work could have already started and finished and marked busy as false. Then you call execute() and busy is marked true (even though it will never again be marked false because the thread has already finished).

    void execute() {
        busy = true;   // What if the thread is already finished?
        task->join();
    }

    void async() {
        busy = true;
        task->detach(); // Never do this.
    }                   // When you are experienced enough to know when
                        // this is useful you will know all the tricks
                        // and not need my code review. But for now
                        // never use this.

Lazy use of handles:

    void initRequest(const std::string &url) {

        // This should only be used after it has already been used.
        // You call this on a brand new handle!
        curl_easy_reset(handle);
    }

Never do a bust wait.

This is a busy wait. Let's assume you fixed the above bug about busy being updated from multiple threads.

    void waitTask() {
        while (busy) {}
    }

This will melt your processor. You should never do a busy wait. Not only will this melt your processor it will waste a CPU doing nothing when it can be actively given to some other task to do something useful.

Suspend the thread until you have work for it to do. Look up condition_variable.

This is dangerous.

If the thread has not completed this will throw an exception. So before you call this you must call waitTask(). That should be made explicit or called from inside cleanTask itself to make sure the thread has finished.

    void cleanTask() {
        if (task != nullptr) {
            delete task;
            task = nullptr;
        }
    }

Answer 2

May be @Loki Astari's comment points out about a major design flaw.

Some minor points from my side for now:

Make the shared static int curlInstanceCount; thread safe

Your static class variable isn't thread safe currently. The easiest way to achieve that would be using a

static std::atomic<int> curlInstanceCount;

instead.

Use consistent style for reference return types

The declaration styles of

 CppUrl& operator= (CppUrl const&);

and other functions like

 CppUrl &get(const std::string &url, ResponseCallback callback) {

are inconsistent, decide for one (I'd personally prefer to attach the & to the type like in the assignment operator declaration).