Skip to content
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

Fix potential deadlocks when building proxies/registering stubs in callbacks from events #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CatalinMihaiGhita
Copy link

The Observer design pattern has now been applied in a way that avoids deadlocks, has better support for concurrency, and provides more natural semantics for the client code:

  • An internal mutex is no longer locked while calling back the notify functors (the
    listeners) registered by the user.

  • The non-interference/concurrency relationship between the notification functions
    and the subscribe/unsubscribe functions has remained valid.

  • A new notification iteration can now begin in parallel (e.g.: on another thread)
    with an on-going notification. The notification iterations do not interfere with
    each other.

  • The functor callbacks registered by the user with calls to the subscribe function
    are now deleted by the call to the unsubscribe function (i.e. deterministically from
    the user's point of view), as opposed to being deleted at some point in the future
    when/if a notification happens to be broadcast again (i.e. nondeterministically
    from the user's point of view).

  • Internally, the implementation of the Event class template has been simplified
    (e.g.: by only using one mutex and by letting go of the subscribe/unsubscribe
    lists) and the code is now easier to understand.

  • Also, an object (of type std::lock_guard) is now used instead of explicit calls to the mutex lock and unlock functions (according to the RAII idiom), resulting in improved safety.

…llbacks from events

The Observer design pattern has now been applied in a way that avoids deadlocks,
has better support for concurrency and provides more natural semantics for the
client code:
- An internal mutex is no longer locked while calling back the notify functors (the
listeners) registered by the user.
- The non-interference/concurrency relationship between the notification functions
and the subscribe/unsubscribe functions has remained valid.
- A new notification iteration can now begin in parallel (e.g.: on another thread)
with an on-going notification. The notification iterations do not interfere with
each other.
- The functor callbacks registered by the user with calls to the subscribe function
are now deleted by the call to the unsubscribe function (i.e. deterministically from
the user's point of view), as opposed to being deleted at some point in the future
when/if a notification  happens to be broadcast again (i.e. nondeterministically
from the user's point of view).
- Internally, the implementation of the Event class template has been simplified
(e.g.: by only using one mutex and by letting go of the subscribe/unsubscribe
lists) and the code is now easier to understand.
@GenivivSOMEIPmaintainer

Thank you for your pull request. We did some testing with it but couldn't accept it like that for CommonAPI3.1.12. The problem is, that it works only with mainloop integration but not if the mainloop integration is not used and the mainloop context is not set. Therefore we have to check if we can use it with some changes that cover also the "non-mainloop" use-case. We have also the question if you have some concrete use cases or tests which fail with the current implementation. The simple use-case to create and to delete proxies in callbacks should work with the current implementation.

@GenivivSOMEIPmaintainer

We will look again at this pull request for CommonAPI 4. At the moment we cannot merge it.

@GenivivSOMEIPmaintainer
Copy link

Issue will be closed when CommonAPI 4 will be released.

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

Successfully merging this pull request may close these issues.

2 participants