Skip to content

Deadlock when accessing smart lock twice within the same expression #2448

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

Closed
gpalmer-latai opened this issue Apr 3, 2025 · 9 comments
Closed

Comments

@gpalmer-latai
Copy link

gpalmer-latai commented Apr 3, 2025

Required information

For the reasons described in #2128 and #2092 we have implemented our own derived version of the PoshRuntime where the segment manager is exposed, like so:

  , segment_manager_(
      m_ipcChannelInterface->getSegmentManagerAddressOffset(), iox::segment_id_t{m_ipcChannelInterface->getSegmentId()})

Whilst updating our fork of Iceoryx to a more recent HEAD, we encountered the problem where suddenly a deadlock is occurring at this line of code.

It turns out to be caused by #2191 which added a smart_lock around m_ipcChannelInterface.

operator-> on smart_lock relies on the RAII behavior of a prvalue Proxy whose lifetime, unfortunately, is bound to that of this statement. This means in effect that two Proxy objects will be created, both of which call lock() on the same mutex in their construct, and neither of which call unlock() until the statement is complete - which does not happen because the second Proxy blocks in the lock() call.

Operating system:
Ubuntu 24

Compiler version:

Clang 14

Eclipse iceoryx version:

main

Observed result or behaviour:

A deadlock when making two calls to a smart_lock object in the same statement

Expected result or behaviour:

The smart lock is unlocked once the operation under -> is completed, thus not causing a deadlock when used twice within the same statement

Conditions where it occurred / Performed steps:

There are a lot of ways this could be triggered, but in particular you can create a class like

struct Baz
{
  int make_int();
};

struct Foo
{
int a,
int b,
};

struct Bar
{
  // This constructor will deadlock
  Bar(iox::concurrent::smart_lock<Baz>& baz) : foo{baz->make_int(), baz->make_int()} {}
  Foo foo;
};

Additional helpful information

@gpalmer-latai
Copy link
Author

Just to be clear - we aren't blocked in any way by this since it was trivial to split up the initialization of that member such that the operator-> don't happen in the same expression.

But I felt that this pitfall warranted calling attention to, if not finding a clever solution for.

@gpalmer-latai
Copy link
Author

It seems like smart_lock<T, MutexType>::operator-> should directly return the pointer to the wrapped type and its implementation, instead of

return Proxy(base, lock);

should be

return Proxy(base, lock).operator->();

@elBoberido
Copy link
Member

@gpalmer-latai it's already late and I have to think this through but I think your proposal would unlock the mutex while one is still operation on the underlying type.

For situation where the wrapped type has to be used multiple times in the same expression, one can use the scope guard

auto g = foo.get_scope_guard();
fubar(g->bla(), g->baz());

But this does not work in your situation. A workaround might be to use an immediately invoked lambda

, segment_manager_(
      [&] { return m_ipcChannelInterface->getSegmentManagerAddressOffset(); } (), ...)

@gpalmer-latai
Copy link
Author

Hm. You are right. operator-> returns a pointer to the underlying type and THEN the operation on that type occurs.

What the ideal behavior would be is that once smart_lock_wrapper->do_thing() completes, the lock is unlocked immediately. But I'm not sure there is a way to express that in C++ without using preprocessor macros.

@gpalmer-latai
Copy link
Author

Well, maybe it is possible with C++26 reflection by essentially generating a wrapper class that mirrors operations on the underlying class with a lock around them.

@elfenpiff
Copy link
Contributor

I think a recursive as underlying mutex for the smart lock solves the problem, see: https://en.cppreference.com/w/cpp/thread/recursive_mutex

@gpalmer-latai I think the usage of this is itself a bug:

Bar(iox::concurrent::smart_lock<Baz>& baz) : foo{baz->make_int(), baz->make_int()} {}

Let's assume it would work like intended (without the deadlock).

  • arguments of functions are evaluated in random order
  • assume another thread modifies baz
  • then foo would acquire two different states of baz

@elBoberido
Copy link
Member

@elfenpiff right, a recursive mutex would solve that problem

@gpalmer-latai you can specify the mutex type with the second template parameter

@elBoberido
Copy link
Member

@gpalmer-latai can this be closed? The smart-lock works as intended and when a immediately invoked lambda or a recursive mutex is used, the problem does not appear.

@gpalmer-latai
Copy link
Author

Sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants