[refactor] Switched httpserver.cpp to use RAII wrapped libevents. #9517

pull kallewoof wants to merge 2 commits into bitcoin:master from kallewoof:raii-httpserver changing 2 files +13 −21

kallewoof commented at 1:52 PM on January 11, 2017: member

Continuation of #9387. I believe this concludes fixing the // XXX RAII places.
in src/httpserver.cpp:None in c7d7ecfa33 outdated
```
 429 | @@ -438,6 +430,8 @@ bool InitHTTPServer()
 430 |      workQueue = new WorkQueue<HTTPClosure>(workQueueDepth);
 431 |      eventBase = base;
 432 |      eventHTTP = http;
 433 | +    base_ctr.release();
 434 | +    http_ctr.release();
```
dcousens commented at 2:09 PM on January 11, 2017:

why the manual release?

kallewoof commented at 2:13 PM on January 11, 2017:

Because eventBase/eventHTTP are global and need them intact. If I don't release they'd be deleted on function return.

dcousens commented at 2:18 PM on January 11, 2017:

I mean, I guess it improves consistency, and the .release(), if you added a comment noting the above about the lifetime, might be clearer than what was there before?

This change seems pointless IMHO, maybe I'm missing something

kallewoof commented at 2:23 PM on January 11, 2017:

Yeah, you're right. Addressing.

kallewoof commented at 2:30 PM on January 11, 2017:

Regarding purpose: the one thing this brings is that no matter how things end, the base and http objects will be properly cleaned up. I have no idea what kind of side-effects an initialized and forgotten event base or evhttp might have. Perhaps none whatsoever.
in src/httpserver.cpp:None in c7d7ecfa33 outdated
```
 401 | @@ -405,17 +402,14 @@ bool InitHTTPServer()
 402 |      evthread_use_pthreads();
 403 |  #endif
 404 |  
 405 | -    base = event_base_new(); // XXX RAII
 406 | -    if (!base) {
 407 | -        LogPrintf("Couldn't create an event_base: exiting\n");
 408 | -        return false;
 409 | -    }
```
dcousens commented at 2:17 PM on January 11, 2017:

~~What happened to the error handling?~~ (https://github.com/bitcoin/bitcoin/blob/master/src/support/events.h#L33)

kallewoof commented at 2:24 PM on January 11, 2017:

In the case of event_base, the wrapper does the error handling already.
kallewoof force-pushed on Jan 11, 2017
fanquake added the label Refactoring on Jan 11, 2017
in src/httpserver.cpp:None in e23ecf7d88 outdated
```
 429 | @@ -438,6 +430,9 @@ bool InitHTTPServer()
 430 |      workQueue = new WorkQueue<HTTPClosure>(workQueueDepth);
 431 |      eventBase = base;
```
laanwj commented at 2:10 PM on January 19, 2017:

What about

eventBase = base_ctr.release(); eventHTTP = http_ctr.release();
kallewoof commented at 2:42 PM on January 19, 2017:

That looks much better. I didn't realize you could do that. Thanks!
kallewoof force-pushed on Jan 19, 2017
dcousens approved
Switched httpserver.cpp to use RAII wrapped libevents. fd369d267b
kallewoof force-pushed on Mar 22, 2017
kallewoof commented at 7:30 PM on March 22, 2017: member

Rebased.
jtimon commented at 6:39 PM on March 23, 2017: contributor

Fast review ACK
Changed event RAII helper functions to inline to deal with duplicate symbol linker errors. 1ae86ec5ec
kallewoof force-pushed on Apr 14, 2017
gmaxwell approved
gmaxwell commented at 4:53 AM on April 14, 2017: contributor

ACK (mostly tested ACK, I'm not so useful for reviewing this stuff. :) )
laanwj commented at 5:16 PM on June 22, 2017: member

utACK 1ae86ec
laanwj merged this on Jun 22, 2017
laanwj closed this on Jun 22, 2017
laanwj referenced this in commit ffce893982 on Jun 22, 2017
kallewoof deleted the branch on Jun 23, 2017
PastaPastaPasta referenced this in commit c2d030675c on Jul 6, 2019
PastaPastaPasta referenced this in commit d77fc906c6 on Jul 8, 2019
PastaPastaPasta referenced this in commit 5125745515 on Jul 9, 2019
PastaPastaPasta referenced this in commit 31f4a602a5 on Jul 11, 2019
barrystyle referenced this in commit 7f978905f6 on Jan 22, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9387 [Refactor] RAII of libevent stuff using unique ptrs with deleters