4
\$\begingroup\$

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();
\$\endgroup\$
  • 3
    \$\begingroup\$ Using multiple threads is the wrong approach. You should be using the async nature of network IO. A single thread can handle thousands of connections simultaneously. If you were doing this manually I would say look at pselect/poll/epoll/libEvent but you are using the libcurl library which already does this. Take a look at Multi Handle \$\endgroup\$ – Martin York Jan 9 '17 at 18:53
5
\$\begingroup\$

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;
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Hi! Sorry for late reply. I just like to clarify some things why I did those, and ask questions altogether: 1. So if I should not be using the std::thread::detach() what is the proper way to do an asynchronous work in C++? 2. I wanted to have a generic exception class, so the name itself MissingHttpMethodException should be enough for the programmer to know what he's missing. You're right, I should be using assert rather than exceptions here. 3. curlInstanceCount is a static variable, and it's not invoking curl_global_init multiple times, base from my tests. \$\endgroup\$ – mr5 Jan 10 '17 at 2:50
  • \$\begingroup\$ 4. Both assignment operator and copy constructor are deleted? Remember, it's a pure header only so there's no implementation of assignment operator. 5. handle = nullptr I did this because I thought an instance of CppUrl might have destroyed first before the underlying std::thread when the user uses an async() function. 6. According to official docs, I should use curl_easy_reset if I wanted to reuse my handle and I think there's no harm if the handle wasn't used before. 7. waitTask() is called before every cleanTask() \$\endgroup\$ – mr5 Jan 10 '17 at 3:03
  • \$\begingroup\$ All in all, thanks for pointing out my flawed design. I just started learning C++ again so somethings are not still clear for me. \$\endgroup\$ – mr5 Jan 10 '17 at 3:03
  • 1
    \$\begingroup\$ @mr5: So it's enough to say that the thread is asynchronous when I didn't invoke the std::thread::join()`. Yes. The thread is already running. When you call join it waits for the thread to finish (but it may have already finished). \$\endgroup\$ – Martin York Jan 10 '17 at 3:46
  • 1
    \$\begingroup\$ @quetzalcoatl Looking at your link it does not provide any gurantees about the ABI across different languages. Only within C++ itself (which is a different language to C). Which is why you have the "extern C" declaration so that on compilers where there is a difference the appropriate code can be generated. \$\endgroup\$ – Martin York Jul 5 at 17:21
2
\$\begingroup\$

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).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.