Fix clang thread safety analysis warnings

[MichaelOrlov]

Workaround for false positive findings by clang thread safety analysis in time controller jump callbacks API.
Address #795

Moved cv.notify_all() inside mutex lock scope, also made method process_callback() as non-static

[MichaelOrlov]

Gist:
https://gist.githubusercontent.com/MichaelOrlov/5fd70f1953b536179399721f25846a73/raw/fc9b346ecfc7d32e68197e58608448b6d2f3a1a9/ros2.repos

BUILD args: --packages-up-to rosbag2_cpp
TEST args: --packages-select rosbag2_cpp
Job: ci_launcher
ci_linux_clang_libcxx ran: https://ci.ros2.org/job/ci_linux_clang_libcxx/5/

[emersonknapp]

Looks good - however I will say that these are not false positives.

• cv was used without the mutex
• A static function does not have access to non-static member variables. The declaration of static void REQUIRES(member_) was ill-formed for requiring a non-static variable.

[MichaelOrlov]

@emersonknapp As regards to the

• cv was used without the mutex

cv.notify_all() should be used when mutex already unlocked. According to the https://en.cppreference.com/w/cpp/thread/condition_variable/notify_all

The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock.

Implemented workaround putting cv.notify_all() under the mutex only works due to the specific optimization implemented in GCC. Tomorrow new type or version of compiler will be used or with some flags turning off this optimization and we will get random errors with missed notifications. I am actually not happy with such type of workarounds.

As regards to the:

A static function does not have access to non-static member variables. The declaration of static void REQUIRES(member_) was ill-formed for requiring a non-static variable.

Not sure if I understand you correctly but process_callback() function doesn't have access to the member variables of the class and purely operates with input parameters.

[MichaelOrlov]

@emersonknapp Oh. BTW fun fact.
I didn't mind to declare process_callback() as static from the very beginning, but clang-tidy gave me a warning that this function could be declared as static and proposed to put static keyword for it. I reviewed this function and come to the conclusion so well why not.

Now we have contradiction between two tools clang-tidy and clang thread safety check :)

[emersonknapp]

cv.notify_all() should be used when mutex already unlocked

That is only according to the documented usage hints, it is not a requirement of the implementation. But, we explicitly said in the code that cv REQUIRES(mutex) - so the TSA is doing exactly what we told it to. It's not a false positive - it's an accurate result of the code that has been written.

Not sure if I understand you correctly but process_callback() function doesn't have access to the member variables of the class and purely operates with input parameters.

The static function was declared REQUIRES(non_static_member_) - there is no way for the static function to have access to that non-static-member. The compiler seems to allow this annotation without failing, but it's logically inconsistent so it would always raise a warning that the mutex is not held.