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

examples/cord_ep: Dead lock when (re-)registering in callback function #12884

Open
MatthiasBraeuer opened this issue Dec 5, 2019 · 1 comment
Assignees
Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: question The issue poses a question regarding usage of RIOT

Comments

@MatthiasBraeuer
Copy link

Description

I was using the cord_ep example and wanted to automatically (re-)register to an resource directory when the cord_ep_standalone_cb_t is executed with the CORD_EP_DEREGISTERED event passed to it.

When I call the register function in sys/net/application_layer/cord/ep/cord_ep.c though, I produce a dead lock, because the function tries to lock a mutex, which is still locked by the function calling the callback function.

Short: cord_ep_standalone thread => cord_ep_update =>
cord_ep_standalone_signal, => application callback => cord_ep_register.

Possible Solutions:

  1. Unlock the mutex before calling cord_ep_standalone_signal (You should do this in all cord_ep functions where cord_ep_standalone_signal is called)
  2. Use a recursive mutex, so that the same thread can lock a mutex multiple times and therefore registering in the callback is possible
  3. Remove the call to cord_ep_standalone_signal within cord_ep, because cord_ep_standalone#_reg_runner is actually calling the callback (Currently the callback is triggered twice when an update fails, which is not wanted behavior anyway).

Solution 3 is probably the best, because it fixes triggering the callback twice and the dead lock issue as well (Do not forget to remove the timer (xtimer_remove(&_timer);) when the update failed, so that the thread will not try to update as long as the device is not (re-)registered).

For all the other functions in cord_ep.c that call cord_eap_standalone_signal, unlocking the mutex before calling it would be probably the best.

Steps to reproduce the issue

  1. In /examples/cord_ep set a callback function (cord_ep_standalone_reg_cb(cord_ep_standalone_cb_t cb)) and register to a resource directory via cord_ep_register(const sock_udp_ep_t *remote, const char *regif) in main
  2. In the callback function call cord_ep_register
  3. Execute the example => you should end up in a dead lock

Expected results

You can call any function of cord_ep in the cord_ep_standalone_cb_t without producing a dead lock.

Actual results

Calling a any function of cord_ep in the cord_ep_standalone_cb_t causes a dead lock.

Versions

@cgundogan cgundogan added Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: question The issue poses a question regarding usage of RIOT labels Dec 5, 2019
@haukepetersen
Copy link
Contributor

Good catch, I actually forgot to document that you should not call any cord_ep functions from within the callback... Also I was able to reproduce the issue that the CORD_EP_DEREGISTERED event is fired twice on a failed update.

And I agree, that the code easily can be improved to make this possible, so lets do that :-)

see #12996

@miri64 miri64 added this to the Release 2020.07 milestone Jul 6, 2020
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: question The issue poses a question regarding usage of RIOT
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants