Fix webserver thread safety #330
Conversation
FlorianChevassu
force-pushed
the
fix_webserver_thread_safety
branch
from
858ef61
to
2f7b4ff
Compare
src/httpserver/webserver.hpp
Outdated
| std::map<details::http_endpoint, http_resource*> registered_resources; | ||
| std::map<std::string, http_resource*> registered_resources_str; | ||
|
|
||
| std::shared_mutex bans_and_allowances_mutex; |
There was a problem hiding this comment.
It might be worth splitting this into two, given the writing use-cases are isolated
src/webserver.cpp
Outdated
| @@ -402,6 +409,7 @@ MHD_Result policy_callback(void *cls, const struct sockaddr* addr, socklen_t add | |||
|
|
|||
| if (!(static_cast<webserver*>(cls))->ban_system_enabled) return MHD_YES; | |||
|
|
|||
| std::shared_lock bans_and_allowances_lock((static_cast<webserver*>(cls))->bans_and_allowances_mutex); | |||
There was a problem hiding this comment.
We can split the if statement here so that we always check only one of the two based on the chosen policy
There was a problem hiding this comment.
In both ACCEPT and REJECT cases, we access bans and allowances, so I don't see how we could only lock one of the mutexes.
I added a commit simplifying the code, with no intended functional change.
There was a problem hiding this comment.
[MINOR] I think the cleanest way would be to have two private methods (is_allowed and is_banned) that use the locks internally and just call into those methods from within here. That should localize the locks to the maximum extent possible.
|
Overall, it looks good - just a few minor comments |
|
It looks like the PR is failing at compiling |
FlorianChevassu
force-pushed
the
fix_webserver_thread_safety
branch
from
101d19f
to
589ff8f
Compare
Sorry about the delay. Should be fixed now. |
Identify the Bug
#329
Description of the Change
This pull request introduces the usage of locks in the werbserver class in order to protect member variables from data races.
Alternate Designs
N/A
Possible Drawbacks
This might slightly increase response time, but should not be noticeable.
Verification Process
N/A
Release Notes
Fixed an issue where multiple threads did access webserver member variables concurrently, leading to data races.