Open FlorianChevassu opened 1 year ago
I am trying to understand the effect of a data race in this case before we add locks, which I am actively trying to keep the main response flow free from.
In both the case of registered resources and ban/unban, the only thing we care about is presence/absence of an entry (not the order of insertion). Isn't thus the worst-case scenario that someone registers a resource and for some time the resource not being registered (or viceversa)? (same for ban/unban). I wonder if all we need to do is to document eventual consistency.
I am much more worried about race conditions instead which can happen if multiple threads are continually registering/unregistering (writing) the same resource (or banning/unbanning the same IP). Kinda insane to do, but I guess it is a legitimate edge case. In this case, I think we should lock on writes so that they don't clash with other writes. Alternatively, I wonder if we should just mark those two specific methods as not thread safe in the documentation.
The underlying std::map
implementation is usually based on red-black trees. This means that an insertion might trigger a re-balancing operation that will modify the internal structure of the tree. Iterating over the map while it is being re-balanced is UB, and might lead to weird issues, not just the "presence/absence" of an entry.
Updating the documentation to clearly state that these methods are not thread safe would implies that the user should be able to have a way to execute those on the same thread that execute the finalize_answer
method. And if I understand correctly, depending on the thread model used, it may be executed on different threads...
sources:
The underlying std::map implementation is usually based on red-black trees. This means that an insertion might trigger a re-balancing operation that will modify the internal structure of the tree. Iterating over the map while it is being re-balanced is UB, and might lead to weird issues, not just the "presence/absence" of an entry.
That is technically incorrect.
See C++ standard guarantees that concurrent non-const accesses to a container are safe if the writes/reads are to different elements of the container. You can see this in 23.1.2/8 and 23.2.2: "The insert members shall not affect the validity of iterators and references to the container, and the erase members shall invalidate only iterators and references to the erased elements".
Specifically for implementation of map (as requirements for the implementation of associative collections) it requires that they don't invalidate existing elements as side effects.
Updating the documentation to clearly state that these methods are not thread safe would implies that the user should be able to have a way to execute those on the same thread that execute the finalize_answer method.
Bar the previous point. I think you are correct that there still is an issue with concurrent modification (insertion/removal) of the same element on both resources.
Because of this, I think your PR is in the right direction if we want to keep the functionalities.
There are of course legitimate use-cases for banning/allowing IPs during the execution of a webserver. I am generally less worried with locking in there as it is more isolated.
I want to investigate with folks using the library on the need of registering (and especially unregistering) resources after a webserver has started. From my limited visibility, that is a feature we might as well lose - making the registered resources structure effectively const after the webserver starts.
See C++ standard guarantees that concurrent non-const accesses to a container are safe if the writes/reads are to different elements of the container.
Yes, concurrently modifying different elements of the container is not an issue, because as you said, iterators are not invalidated. But the fact that iterators are not invalidated only means that you can safely use/modify the value itself, not that you can safely increment it.
The standard provides an exhaustive list of containers non-const member functions that can be called concurrently without creating data races, and insert
and remove
are not part of this list.
As said above, data races are not an issue. Race conditions are. Data races can indeed happen (as you say), but they don't matter in the use case of this library. Race conditions would be a problem, and that's what I am saying the standard prevents from happening.
In any case, this is beyond the point given that modification of the same element is still possible by the current interface, so not even worth debating further.
From my chat with a few clients, there seem to be some legitimate use cases to keep the functionality of adding/removing resources while the service execute. As such, your PR seems to be going in the right direction. I'll leave you specific comments there.
Prerequisites
Description
The documentation states that
All functions are guaranteed to be completely reentrant and thread-safe (unless differently specified)
. However, thewebserver
implementation does not use any protection while reading/modifying its member variables (registered_resources
,registered_resources_str
,bans
andallowances
).Steps to Reproduce
Here is a small test that show the issue when compiled using clang-16 with thread sanitizing enabled:
Expected behavior:
No data races are detected by clang.
Actual behavior: Clang reports the following data race (and others):
Reproduces how often: 100%
Versions