http: Do a pending c++11 simplification handling work items #7966

laanwj commented at 5:07 PM on April 28, 2016: member

Use std::unique_ptr for handling http work items, and std::move to move them into and out of the queue.

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.

laanwj added the label RPC/REST/ZMQ on Apr 28, 2016

in src/httpserver.cpp:None in c0c7d8b7d7 outdated

  70 | @@ -71,8 +71,7 @@ class WorkQueue
  71 |      /** Mutex protects entire object */
  72 |      CWaitableCriticalSection cs;
  73 |      CConditionVariable cond;
  74 | -    /* XXX in C++11 we can use std::unique_ptr here and avoid manual cleanup */
  75 | -    std::deque<WorkItem*> queue;
  76 | +    std::deque<std::unique_ptr<WorkItem> > queue;

sipa commented at 5:22 PM on April 28, 2016:

Also, in C++11, you no longer need to avoid the '>>' in Template1<Template2<Template3>>.

paveljanik commented at 5:28 PM on April 28, 2016:

I was just about to ask why the space is there...

laanwj commented at 7:15 PM on April 28, 2016:

@paveljanik because >> is an operator. But yes with c++11 this is apparently parsed differently so will remove the space.

laanwj force-pushed on Apr 28, 2016

in src/httpserver.cpp:None in 3fc99f4d9b outdated

 104 | @@ -106,19 +105,15 @@ class WorkQueue
 105 |       */
 106 |      ~WorkQueue()

paveljanik commented at 8:12 PM on April 28, 2016:

Do we need an empty destructor?

laanwj commented at 11:10 AM on May 2, 2016:

I'll remove it.

laanwj commented at 9:59 AM on May 4, 2016:

On second thought, I'm going to keep it, the precondition comment is still relevant.

in src/httpserver.cpp:None in 3fc99f4d9b outdated

 287 |          assert(workQueue);
 288 | -        if (workQueue->Enqueue(item.get()))
 289 | -            item.release(); /* if true, queue took ownership */
 290 | -        else
 291 | +        if (!workQueue->Enqueue(std::move(item)))
 292 |              item->req->WriteReply(HTTP_INTERNAL, "Work queue depth exceeded");

kazcw commented at 9:20 PM on April 28, 2016:

item has been given to the queue here

theuni commented at 9:21 PM on April 28, 2016:

use after move here

laanwj commented at 11:06 AM on May 2, 2016:

@kazcw @theuni Bah, good catch. I suppose there's no such thing as a conditional move? Maybe it could return the unique_pointer ("move back") if it couldn't be pushed into the queue? The other option would be to move the error message into the queue but that'd not be very nice.

kazcw commented at 3:44 PM on May 2, 2016:

The actual moving happens in unique_ptr's ctor. The function just needs to accept a unique_ptr&&, then it can freely move that into a new unique_ptr or not.

laanwj commented at 10:01 AM on May 4, 2016:

@kazcw I've updated to do that. Can you please re-review to see if this was what you meant?

laanwj commented at 12:41 PM on May 4, 2016:

It doesn't seem to work. I've tested with a (hacked) work-queue size of 0, and the connection is closed before the error message can be sent.

laanwj commented at 1:01 PM on May 4, 2016:

Leaving out the std::move in the ->Enqueue line gives a compile error about pointer conversions:

/home/orion/projects/bitcoin/bitcoin/src/httpserver.cpp:289:33: error: no viable conversion from 'unique_ptr<HTTPWorkItem>' to 'unique_ptr<HTTPClosure>'
        if (!workQueue->Enqueue(item))

Bleh.

laanwj force-pushed on May 4, 2016

laanwj commented at 1:33 PM on May 4, 2016: member

Decided to forget about using std::move to move items conditionally into the queue, the resulting code was too complex, the old code is cleaner.

laanwj force-pushed on May 4, 2016

in src/httpserver.cpp:None in 867bc6d382 outdated

 111 | @@ -118,7 +112,7 @@ class WorkQueue
 112 |          if (queue.size() >= maxDepth) {
 113 |              return false;
 114 |          }
 115 | -        queue.push_back(item);
 116 | +        queue.push_back(std::unique_ptr<WorkItem>(item));

theuni commented at 5:49 AM on May 5, 2016:

nit: queue.emplace_back(item);

theuni commented at 5:49 AM on May 5, 2016: member

ut ACK, other than the little nit

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.

091d6e0499

http: Add log message when work queue is full

More useful error reporting.

f97b410fdd

http: Change boost::scoped_ptr to std::unique_ptr in HTTPRequest

No need for boost here.

37b21372a0

http: use std::move to move HTTPRequest into HTTPWorkItem

Thanks to Cory Fields for the idea.

f0188f9178

laanwj force-pushed on May 5, 2016

laanwj commented at 10:56 AM on May 5, 2016: member

Fixed @theuni's nit, thanks

laanwj merged this on May 5, 2016

laanwj closed this on May 5, 2016

laanwj referenced this in commit d51618e481 on May 5, 2016

in src/httpserver.cpp:None in f0188f9178

 111 | @@ -118,7 +112,7 @@ class WorkQueue
 112 |          if (queue.size() >= maxDepth) {
 113 |              return false;
 114 |          }
 115 | -        queue.push_back(item);
 116 | +        queue.emplace_back(std::unique_ptr<WorkItem>(item));

theuni commented at 3:57 PM on May 5, 2016:

Creating the unique_ptr here defeats the purpose of the emplace :) Passing it item directly would mean that the unique_ptr would be constructed in-place. As is, it's moved instead.

Not worth worrying about, just pointing it out.

laanwj commented at 5:02 PM on May 5, 2016:

Yes by all means feel free to improve this further.

codablock referenced this in commit d4dc6db432 on Sep 16, 2017

codablock referenced this in commit 0cd1e2ce65 on Sep 19, 2017

codablock referenced this in commit 33233409a9 on Dec 21, 2017

sickpig referenced this in commit 46b5b86be5 on Mar 9, 2018

sickpig referenced this in commit dadc2880a1 on Mar 9, 2018

sickpig referenced this in commit 6d4a55878a on Mar 12, 2018

sickpig referenced this in commit 6db640a393 on Mar 12, 2018

sickpig referenced this in commit 76cf243311 on Mar 12, 2018

sickpig referenced this in commit 4428056252 on Mar 12, 2018

marlengit referenced this in commit 297ac08c3a on Sep 25, 2018

zkbot referenced this in commit faf3825eed on Sep 30, 2020

zkbot referenced this in commit a983344931 on Oct 1, 2020

DrahtBot locked this on Sep 8, 2021