Improve locked queue deadlocks

[julianoes]

This is a further attempt following #590 to prevent potential deadlocks.

• mavlink_commands: remove state variable and mutex.
• mavlink_parameters: remove state variable and mutex.
• locked_queue: avoid re-locking on pop (always require borrow first).

[JonasVautherin]

@hdiethelm: I would love to get your review on that, if you have time! :-)

[JonasVautherin]

@julianoes Should this be a [WIP] PR?

[hdiethelm]

Oh, quite a lot of changes! I don't have the time to review all, sorry. I just looked a bit trough and so far it looks good!

The only risk is that somewhere you do a borrow_front and forget the corresponding pop_front/return_front resulting also in a deadlock.

I was thinking we could also drop locked_queue completely and instead use a normal queue as it is. Then add a mutex for each queue directly in MAVLinkParameters and MAVLinkCommands and lock it using lock_guard or unique_lock so unlocking is guaranteed. The disadvantage is if you forget to lock the mutex, you could mess up the queue.

I have also an other idea, I'll just test if it works.

[hdiethelm]

Check this one:
hdiethelm@f33b63d
It should fix the potential problem of double borrow and missed pop/return. But I just tested it using the unit tests.

[hdiethelm]

I added a test and it works:
hdiethelm@e986d17
By the way: Is there a way to catch intentionally generated asserts? I tested also the double_borrow from the same process and pop / return without borrow case and the test fails with assert as it should... ;-) However, the test runner is killed which is not so nice.

[julianoes]

So, yes we can test for an assert using EXPECT_DEATH(call(), "assert bla failed...").
But one problem is that the test will fail for Release builds where asserts are not built.

[hdiethelm]

What do you think about my suggestion, passing a unique_lock with no associated mutex to borrow_front() to avoid an accidental lock without unlock?
hdiethelm@f33b63d and hdiethelm@e986d17

[julianoes]

What do you think about my suggestion, passing a unique_lock with no associated mutex to borrow_front() to avoid an accidental lock without unlock?

Thanks yes! I think that's definitely better because it can avoid mistakes. I was hoping that we can have the locked queue pass some kind of smart pointer thing that keeps the lock internal and returns if it goes out of scope. I didn't have time to try that though.

[hdiethelm]

What do you think about my suggestion, passing a unique_lock with no associated mutex to borrow_front() to avoid an accidental lock without unlock?

Thanks yes! I think that's definitely better because it can avoid mistakes. I was hoping that we can have the locked queue pass some kind of smart pointer thing that keeps the lock internal and returns if it goes out of scope. I didn't have time to try that though.

unique_lock is one way of that kind of smart pointer.
However, you could also write some kind of queue_guard class which calls return_front in it's destructor. Same effect, somewhat nicer look.
So it would be something like:

{
    queue_guard guard(locked_queue);
    locked_queue.borrow_front(gard);
    //No return_front needed, ~queue_guard handles this
}

My way is:

{
    std::unique_lock<std::mutex> queue_lock;
    locked_queue.borrow_front(queue_lock);
    //No return_front needed, ~unique_lock unlocks the mutex
}

[julianoes]

I gave this a try to get to something close to the guard semantics:

auto guard = _work_queue.guard();
auto work = guard.get_front();

if (bla) {
    guard.pop_front();
}

See the last 3 commits. What do you think?

[hdiethelm]

Looks nice! I think that should do it!

[hdiethelm]

locked_queue: fix MSVC Debug unit test
That was something i was also thinking about.

Guard guard()
{
        Guard new_guard{_mutex, _queue};
        return new_guard;
}

is not so nice.

QueueGuard guard(_work_queue);
auto work = guard.get_front();
if (bla) {
    guard.pop_front();
}

could be better. And then overwrite the copy constructor of QueueGuard = delete, so it can not be copied as it is done in unique_lock.

Edit: added ``` around code.

[julianoes]

QueueGuard guard(_work_queue);

Ah, I didn't think of that, yes that would be nicer!

[hdiethelm]

Nice!

[julianoes]

Just reviewed this myself. I think it's all correct.

[julianoes]

I would appreciate a review @JonasVautherin or @hdiethelm. @hdiethelm I made you a collaborator if you want to be involved 😄

[JonasVautherin]

Spent some time reviewing that, and as far as I can tell, it looks good to me.

[julianoes]

Awesome, thanks @JonasVautherin. I'll rebase and merge.