Skip to content

Commit d75c8b8

Browse files
committed
http: refactor: use encapsulated HTTPRequestTracker
Introduces and uses a HTTPRequestTracker class to keep track of how many HTTP requests are currently active, so we don't stop the server before they're all handled. This has two purposes: 1. In a next commit, allows us to untrack all requests associated with a connection without running into lifetime issues of the connection living longer than the request (see bitcoin#27909 (comment)) 2. Improve encapsulation by making the mutex and cv internal members, and exposing just the WaitUntilEmpty() method that can be safely used.
1 parent 9d5150a commit d75c8b8

File tree

1 file changed

+74
-15
lines changed

1 file changed

+74
-15
lines changed

src/httpserver.cpp

Lines changed: 74 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <rpc/protocol.h> // For HTTP status codes
1818
#include <shutdown.h>
1919
#include <sync.h>
20+
#include <util/check.h>
2021
#include <util/strencodings.h>
2122
#include <util/threadnames.h>
2223
#include <util/translation.h>
@@ -26,6 +27,7 @@
2627
#include <cstdlib>
2728
#include <deque>
2829
#include <memory>
30+
#include <numeric>
2931
#include <optional>
3032
#include <string>
3133
#include <unordered_set>
@@ -149,10 +151,73 @@ static GlobalMutex g_httppathhandlers_mutex;
149151
static std::vector<HTTPPathHandler> pathHandlers GUARDED_BY(g_httppathhandlers_mutex);
150152
//! Bound listening sockets
151153
static std::vector<evhttp_bound_socket *> boundSockets;
154+
155+
/**
156+
* @brief Helps keep track of open `evhttp_connection`s with active `evhttp_requests`
157+
*
158+
*/
159+
class HTTPRequestTracker
160+
{
161+
private:
162+
mutable GlobalMutex m_mutex;
163+
mutable std::condition_variable m_cv;
164+
//! For each connection, keep a counter of how many requests are open
165+
std::unordered_map<const evhttp_connection*, size_t> m_tracker GUARDED_BY(m_mutex);
166+
167+
void RemoveConnectionInternal(const evhttp_connection* conn) EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
168+
{
169+
auto it{m_tracker.find(Assert(conn))};
170+
if (it != m_tracker.end()) {
171+
m_tracker.erase(it);
172+
if (m_tracker.empty()) m_cv.notify_all();
173+
}
174+
}
175+
public:
176+
//! Increase request counter for the associated connection by 1
177+
void AddRequest(evhttp_request* req)
178+
{
179+
const evhttp_connection* conn{Assert(evhttp_request_get_connection(Assert(req)))};
180+
LOCK(m_mutex);
181+
auto it{m_tracker.find(conn)};
182+
if (it == m_tracker.end()) {
183+
// new connection, initialize counter to 1
184+
m_tracker[conn] = 1;
185+
} else {
186+
++(it->second);
187+
}
188+
}
189+
//! Decrease request counter for the associated connection by 1, remove connection if counter is 0
190+
void RemoveRequest(evhttp_request* req)
191+
{
192+
const evhttp_connection* conn{Assert(evhttp_request_get_connection(Assert(req)))};
193+
LOCK(m_mutex);
194+
auto it{m_tracker.find(conn)};
195+
if(it != m_tracker.end() && it->second > 0) {
196+
if(--(it->second) == 0) RemoveConnectionInternal(conn);
197+
}
198+
}
199+
//! Remove a connection entirely
200+
void RemoveConnection(const evhttp_connection* conn)
201+
{
202+
WITH_LOCK(m_mutex, RemoveConnectionInternal(conn));
203+
}
204+
205+
size_t CountActiveRequests() const
206+
{
207+
LOCK(m_mutex);
208+
return std::accumulate(m_tracker.begin(), m_tracker.end(), size_t(0),
209+
[](size_t acc_count, const auto& pair) { return acc_count + pair.second; });
210+
}
211+
size_t CountActiveConnections() const { return WITH_LOCK(m_mutex, return m_tracker.size()); }
212+
//! Wait until there are no more connections with active requests in the tracker
213+
void WaitUntilEmpty() const
214+
{
215+
WAIT_LOCK(m_mutex, lock);
216+
m_cv.wait(lock, [this]() { return CountActiveConnections() == 0; });
217+
}
218+
};
152219
//! Track active requests
153-
static GlobalMutex g_requests_mutex;
154-
static std::condition_variable g_requests_cv;
155-
static std::unordered_set<evhttp_request*> g_requests GUARDED_BY(g_requests_mutex);
220+
static HTTPRequestTracker g_requests;
156221

157222
/** Check if a network address is allowed to access the HTTP server */
158223
static bool ClientAllowed(const CNetAddr& netaddr)
@@ -210,14 +275,11 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m)
210275
/** HTTP request callback */
211276
static void http_request_cb(struct evhttp_request* req, void* arg)
212277
{
213-
// Track requests and notify when a request is completed.
278+
// Track active requests
214279
{
215-
WITH_LOCK(g_requests_mutex, g_requests.insert(req));
216-
g_requests_cv.notify_all();
280+
g_requests.AddRequest(req);
217281
evhttp_request_set_on_complete_cb(req, [](struct evhttp_request* req, void*) {
218-
auto n{WITH_LOCK(g_requests_mutex, return g_requests.erase(req))};
219-
assert(n == 1);
220-
g_requests_cv.notify_all();
282+
g_requests.RemoveRequest(req);
221283
}, nullptr);
222284
}
223285

@@ -473,13 +535,10 @@ void StopHTTPServer()
473535
}
474536
boundSockets.clear();
475537
{
476-
WAIT_LOCK(g_requests_mutex, lock);
477-
if (!g_requests.empty()) {
478-
LogPrint(BCLog::HTTP, "Waiting for %d requests to stop HTTP server\n", g_requests.size());
538+
if (g_requests.CountActiveConnections() != 0) {
539+
LogPrint(BCLog::HTTP, "Waiting for %d requests to stop HTTP server\n", g_requests.CountActiveRequests());
479540
}
480-
g_requests_cv.wait(lock, []() EXCLUSIVE_LOCKS_REQUIRED(g_requests_mutex) {
481-
return g_requests.empty();
482-
});
541+
g_requests.WaitUntilEmpty();
483542
}
484543
if (eventHTTP) {
485544
// Schedule a callback to call evhttp_free in the event base thread, so

0 commit comments

Comments
 (0)