-
Notifications
You must be signed in to change notification settings - Fork 669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Standardize thread-safety documentation #1004
Comments
FYI @ivanpauno |
Also, @ros2/team to kickstart the discussion. |
isn't this exactly the definition of re-entrant?
This will usually only matter if the same object is passed to different functions of the group concurrently (you can also have problems if they share global state, but we probably want to avoid that). I would also like to differentiate the case of "it's safe to call if concurrently" from "concurrent calls have some sort of memory ordering ensured". |
We should take an account why we're documenting this:
IMO if differentiating some of this cases will help rmw implementations and client libraries to get thread safety right, being more specific does worth it. |
Whoops, yeah. Updated the description.
We should do our best to avoid that in
That's a good point. We should also define what we mean by safe. Program termination isn't the worst prospect. Races and contentions breaking caller expectations are much harder to track and fix. IMHO if we want to make thread-safety an API requirement (and I think we have to, to some extent), we have to be specific. Also, the less unnecessary synchronization we do in upper layers the better the overall performance will be. |
This sounds like it could be really hard to track. I guess it would depend on these groups being very well specified? A better angle might be to talk about thread-safe data structures rather than whether a group of functions can operate concurrently on a single instance. Do you have an example of where you see this being used? For example the context structure when used in any of the |
+1 to making it a requirement. |
I think that @hidmic comment was going in that direction. |
What I meant was talking about data structures that contain their own locking or similar mechanisms to ensure they are thread safe, rather than putting the onus on the functions to declare that they use the data structure safely.
I don't understand what this means. Do you mean something like "all functions that are thread-safe over data structure X only ever read it"? That would imply that even though those functions are thread safe over X, you cannot use them at the same time as functions that write X or you would lose that thread safety. On the other hand, if you mean "this function uses the locks to manage its access to X" then I think that you may as well just talk about whether X has associated locks or not, and make all functions that touch X thread-safe with respect to X by using the locks. Anything that doesn't use the locks would be broken rather than just "not thread safe". |
This makes sense to me. It'd seem to me that if we document (a) data structures' support for concurrent access (or lack thereof), and (b) functions' reentrancy and argument mutability (not always implied by the arguments' type, e.g. allocators), we would be painting a complete picture.
I will say that knowing when a read-write lock can be used instead of a mutex would be valuable.
Agreed. |
👍 Reconsidering my previous comment, I agree with this. |
@ros2/team ping on this one for any other ideas or opinions here. |
ros2/rmw#270 introduced extra paragraphs to explain thread safety of some functions, which I think is nice. I think that would be enough to have thread-safety clearly documented. |
Feature request
Feature description
Often, though not consistently, ROS 2 APIs explicitly state their thread (un)safety. These binary statements, when not qualified by a footnote (and even then), are quite unspecific. There's no mention as to:
As a result, we have a vague qualifier for authors, reviewers, and users.
There have been some exchanges in the past, but still it creeps up every now and then. I think we should sit and unambiguously standardize how we document thread-safety.
Implementation considerations
It's worth exploring how other software standards and libraries do this.
For POSIX:
Functions like
memset
orfree
are tagged asMT-Safe
-- that is, no guarantees are made when using these functions' arguments concurrently with the same functions or others.For Qt:
Perhaps we should talk about reentrant functions and thread-safe data structures? Maybe we should state if reads or writes are to be expected on any given argument?
The text was updated successfully, but these errors were encountered: