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
|
|
||
| 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. |
|
Superseded by #350 - I rebased your branch onto master to resolve conflicts and pick up CI fixes. Thanks for the contribution! |
3 participants
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.