local tracking for subscription_count concern

[BrannonKing]

I'm opening this for local tracking of the issue here: ros2/ros2#352

I'm not sure if it concerns rmw_coredx or not; I'm not sure how the subscription array is being managed.

[BrannonKing]

In a similar vein, the wait method at the bottom of functions.hpp does not detach conditions for services and clients. Is that by design?

[ClarkTucker]

No, not by design... looks like the detach_condition() call should be added to the services and clients blocks.

[BrannonKing]

In that same method, I find this code (repeated in several places) kind of scary:

    // if client condition is not found in the active set
    // reset the subscriber handle
    if (!(j < active_conditions.size())) {
      clients->clients[i] = 0;
    }

Isn't that a pointer being set to null? Who handles the cleanup on that object? And just before that code is an O(n) search (repeated in there several places) that seems it could be replaced by a comparison on the detach_condition return value (if it's needed at all). This wait method is hit very frequently; it's worth some optimization.

[ClarkTucker]

The calling code uses the pointers in the array, after calling rmw_wait(), as an indicator of which one[s] triggered their condition. Setting the entry to null removes the entry from further consideration. The caller must maintain the full set of subscriptions, services, etc, so there should be no concern for memory leak...
See rmw.h for some documentation...

[BrannonKing]

To be clear, @ClarkTucker , are you planning to put in a fix for the missing detach_condition() calls or do you want a pull request?

Second, to close this out, I need to understand if you agree that the places in functions.hpp where we iterate with a for loop using subscriber_count, publisher_count, services_count, etc. should just continue instead of returning an error when the item is null. I need this to run for days at a time without crashing. I haven't dug through the code that (de)allocates those items to know if it is all thread-safe, if those lists are never modified while they are iterated in functions.hpp, if items are removed vs. nulled, etc.

[ClarkTucker]

Yes, I'll merge the missing detach_conditions(). [just done]

I don't feel qualified to answer your questions concerning the behavior of the code that calls rmw_coredx::wait(). We simply mimicked the other rmw implementations...