ros2
Remove suspicious wait set locking code
The way I read it, inuse is only ever used with CddsWaitset::lock held, so in that sense, I don't see a problem. However, merely accessing inuse correctly is not the same thing as it being used correctly, and there you may well have a point.
It serves a dual purpose: the first is to prevent concurrent calls to wait on a single wait set. The DDS standard forbids this (even if Cyclone doesn't) and so surely layers above RMW can't be using the same waitset from multiple threads at the same time. That test is therefore merely a sanity check.
Preventing this is useful because it guarantees that no-one is mucking about with the waitset while it is blocked in rmw_wait, allowing one to attach the entities in a very specific order and caching ranks in this order. This way, on return from dds_waitset_wait, it can relatively efficiently null out any entries that did not trigger. (The rmw_wait interface really is a disastrously bad one — and one that the Unix select call also suffers from and that has prompted umpteen different interface to try and avoid it …)
Secondly, there is the caching of attached entries — the assumption that the set of subscriptions, clients, services, guard conditions and events will usually not change from call to call, and that for realistic use-cases, it is cheaper to do compare arrays on entry than it is to attach/detach each entry every call. That's based on an experiment I once did (must've been a microbenchmark, like a hacked talker/listener pair). I would be good to re-do that check: the price paid in complexity is not insignificant.
The worst part of the complexity is having some rather delicate dependencies between entities, waitsets and behaviour of rcl. That's also the source of the "I'm assuming one is not allowed to delete an entity while it is still being used ..." comment in clean_waitset_caches, which states an unverified assumption on the allowed behaviours of rcl. It may be a reasonable assumption given that between rcl and rmw there's nothing but pointers, but that doesn't necessarily mean it is true.
The change you propose has an issue of itself as well: by extending the scope of CddsWaitset::lock across the call to dds_waitset_wait, it will make clean_waitset_caches block on calls to rmw_wait, and so delay the deleting of subscriptions, &c. That could be worked around by adding a guard condition for causing a spurious wakeup (and so on and so forth), but that's trading one piece of horror for another.
In short, I'm not quite ready to accept your PR. But if you'd have the ability to do a bit of measuring the cost of doing attach/detach for each call vs this caching strategy, that'd be great. Then we can either throw it away or we'll at least have decent evidence of the cost.
The issue I see is that the lock is only held for a short time, so it doesn’t prevent threads from using the same waitset. And I think the compiler can optimize out the write to inuse in a way that other threads never see it.
~~Will modify~~ now modified to prevent deadlock
I want to make sure I understand your aside "(The rmw_wait interface really is a disastrously bad one — and one that the Unix select call also suffers from and that has prompted umpteen different interface to try and avoid it …)".
The idea behind waitsets seems to be "I'm interested in when one of these conditions becomes true. I don't care which one" and having to iterate through (and god forbid poll) all the events is expensive. This is compared to an event queue, where, when a condition becomes true, that change becomes immediately visible with minimal work to see what has changed. I assume this is what "attaching" a condition to the waitset with dds_waitset_attach is basically doing - constructing that event queue.
And the specific horror of the rmw_wait function is that there's no way to tell, besides looking at all its arguments, whether a waitset has changed from one call to the next. Having an API that passes a waitset object and informs the rmw layer whenever it needs to change that waitset would be more to your liking. Actually, it looks like it should be able to do this in theory with the current API, except that Executor::wait_for_work empties and re-populates the waitset each time, even if it is actually unchanged.
Yes, although I wouldn't call it an event queue: you wait until any of the attached conditions become true, and the idea is that the set of conditions you want to wait changes only rarely. On return, you get a list of all conditions that triggered. Typically it's very few of them, but there can be multiple.
The spec'd DDS API then returns the objects that triggered; Cyclone DDS instead returns an associated intptr_t to be used as one sees fit. Object handles in Cyclone are positive int32_ts, so can trivially be put in, but one can also use numbers that mean something, or even pointers. The idea is that you can, e.g., write a loop:
struct closure { ... };
{ struct closure *c = malloc(sizeof(*c)); c->func = mycallback; c->arg = myarg; }
dds_waitset_attach(ws, reader, (intptr_t) c);
nxs = dds_waitset_wait(ws, xs, lengthof(xs), timeout);
for (int32_t i = 0; i < nxs; i++) {
struct closure *c = (struct closure *) xs[i];
c->func (c->arg);
}
but this is definitely something that is not directly supported by the spec'd DDS API.
And all this matches exactly with what the upper layers of rcl are trying to achieve ... create a waitset, add some objects to it with callbacks you want to invoke when there's some work to do, then invoke them.
But instead of building on a sensible primitive (even the one in the DDS API is sensible, all it takes is a small map to locate the callback given the object pointer), it constructs that waitset object, deconstructs it into arrays of pointers to objects, passes those to rmw_wait for it to erase the pointers that did not trigger, then scans the arrays for the non-null entries, &c.
The RMW layers therefore have to assume the set of pointers can change from call to call; and one further has to assume that memory may be reused (hence the need for clean_waitset_caches).
Literally all of it could have been skipped ...
A good cleaning up would be valuable, but that amounts to a pretty massive rewrite of the ROS2 core executor code, so I guess it's not going to happen. Which leaves as the only realistic option extending the RMW interface to inform the lower layer of the attach/detach events.