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

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 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;
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;
// busy = false;
});
return *this;
}

/**
* Perform a request with a POST method
* @param const std::string &url: URL to be requested
* @param s

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 g

StackExchange Code Review Q#152132, answer score: 6