From 091d6e04998d2c88dd7f42ad2d90929f428764c2 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 28 Apr 2016 19:03:05 +0200 Subject: [PATCH 1/4] http: Do a pending c++11 simplification Use std::unique_ptr for handling work items. This makes the code more RAII and, as mentioned in the comment, is what I planned when I wrote the code in the first place. --- src/httpserver.cpp | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index a98eff7c1..8297b6f75 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -71,8 +71,7 @@ private: /** Mutex protects entire object */ CWaitableCriticalSection cs; CConditionVariable cond; - /* XXX in C++11 we can use std::unique_ptr here and avoid manual cleanup */ - std::deque queue; + std::deque> queue; bool running; size_t maxDepth; int numThreads; @@ -101,15 +100,11 @@ public: numThreads(0) { } - /*( Precondition: worker threads have all stopped + /** Precondition: worker threads have all stopped * (call WaitExit) */ ~WorkQueue() { - while (!queue.empty()) { - delete queue.front(); - queue.pop_front(); - } } /** Enqueue a work item */ bool Enqueue(WorkItem* item) @@ -118,7 +113,7 @@ public: if (queue.size() >= maxDepth) { return false; } - queue.push_back(item); + queue.emplace_back(std::unique_ptr(item)); cond.notify_one(); return true; } @@ -127,18 +122,17 @@ public: { ThreadCounter count(*this); while (running) { - WorkItem* i = 0; + std::unique_ptr i; { boost::unique_lock lock(cs); while (running && queue.empty()) cond.wait(lock); if (!running) break; - i = queue.front(); + i = std::move(queue.front()); queue.pop_front(); } (*i)(); - delete i; } } /** Interrupt and exit loops */ From f97b410fdd57743f9a26ea49086534000eb9626a Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 4 May 2016 15:37:26 +0200 Subject: [PATCH 2/4] http: Add log message when work queue is full More useful error reporting. --- src/httpserver.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 8297b6f75..67a9e1e92 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -286,8 +286,10 @@ static void http_request_cb(struct evhttp_request* req, void* arg) assert(workQueue); if (workQueue->Enqueue(item.get())) item.release(); /* if true, queue took ownership */ - else + else { + LogPrintf("WARNING: request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting\n"); item->req->WriteReply(HTTP_INTERNAL, "Work queue depth exceeded"); + } } else { hreq->WriteReply(HTTP_NOTFOUND); } From 37b21372a0a3e5d876a87f03c8cbef2e02d8dc92 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 4 May 2016 15:55:23 +0200 Subject: [PATCH 3/4] http: Change boost::scoped_ptr to std::unique_ptr in HTTPRequest No need for boost here. --- src/httpserver.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 67a9e1e92..c193d2af1 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -36,7 +36,6 @@ #include // for to_lower() #include -#include /** Maximum size of http request (request line + headers) */ static const size_t MAX_HEADERS_SIZE = 8192; @@ -54,7 +53,7 @@ public: func(req.get(), path); } - boost::scoped_ptr req; + std::unique_ptr req; private: std::string path; From f0188f9178a22fd493ed228c008d4cc25ac2952d Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 4 May 2016 16:05:17 +0200 Subject: [PATCH 4/4] http: use std::move to move HTTPRequest into HTTPWorkItem Thanks to Cory Fields for the idea. --- src/httpserver.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index c193d2af1..812940eaf 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -44,8 +44,8 @@ static const size_t MAX_HEADERS_SIZE = 8192; class HTTPWorkItem : public HTTPClosure { public: - HTTPWorkItem(HTTPRequest* req, const std::string &path, const HTTPRequestHandler& func): - req(req), path(path), func(func) + HTTPWorkItem(std::unique_ptr req, const std::string &path, const HTTPRequestHandler& func): + req(std::move(req)), path(path), func(func) { } void operator()() @@ -281,7 +281,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg) // Dispatch to worker thread if (i != iend) { - std::unique_ptr item(new HTTPWorkItem(hreq.release(), path, i->handler)); + std::unique_ptr item(new HTTPWorkItem(std::move(hreq), path, i->handler)); assert(workQueue); if (workQueue->Enqueue(item.get())) item.release(); /* if true, queue took ownership */