a deadlock when janus_videoroom_destroy_session

[mivuDing]

Hi, I found a possible deadlock when "publisher A" enter janus_videoroom_destroy_session, and "subscriber B" subscribe "publisher A" at the same time. The detail step can be seen by the attach.

How to solve the problem?I want to unlock the sessions_mutex before destroy publisher, is it OK?

Could you do me a favor? Thanks very much!

[lminiero]

Good catch, that's indeed a nasty deadlock that could happen under rare circumstances, due to the fact that the mutexes may be locked in different order there. Your proposed fix sounds reasonable to me, since the main purpose of sessions_mutex is protecting the sessions hashtable, and other resources should be protected by other locks after that. If you found an easy way to replicate the issue, you can check if you notice any regression there.

Of course, a pull request with this fix would be welcome, especially since it would give you proper credit for the patch! If you want me to do the commit instead, please let me know.

[mivuDing]

Thanks for your reply and suggestions! It is very hard to replicate the issue! The janus server is running nearly one month, and the deadlock only occurred twice.

I will take my proposed method and observe it for a period of time. I would tell you the result if it really take effect and has no side effect.

Of course if you get a good ideas, it is my great pleasure if you let me know, thank you very much!

[mp16]

It can explain my problem described here #1974 (comment) few days ago, where Janus is still accepting connection, but no more data are send.

Since Janus run with gdb, and since I had this bug few minutes ago, you gave me the idea to dump all threads: https://pastebin.com/xTgfkRjA (built from commit 9386afb)

47 threads are stuck in janus_videoroom_hangup_media:

janus-gateway/plugins/janus_videoroom.c

Line 5138 in 9386afb

janus_mutex_lock(&sessions_mutex);

6 threads are stuck in janus_videoroom_slow_link:

janus-gateway/plugins/janus_videoroom.c

Line 4990 in 9386afb

janus_mutex_lock(&sessions_mutex);

1 thread is stuck in janus_videoroom_handler:

janus-gateway/plugins/janus_videoroom.c

Line 5633 in 9386afb

janus_mutex_lock(&sessions_mutex);

1 thread is stuck in janus_videoroom_create_session:

janus-gateway/plugins/janus_videoroom.c

Line 2405 in 9386afb

janus_mutex_lock(&sessions_mutex);

I think it's related to this issue because it's also in the videoroom plugin, and related to the session mutex.

So, a fix is more than welcome because I had the issue friday, I have it also today... it starts to be anoying. I hope that you will push soon.

[lminiero]

@mp16 a fix has already been suggested by @mivuDing (check his last image), so you can try to apply it manually in your environment to check if you notice any issues/regressions.

[mp16]

I have seen that image, but since I don't really know the code, it's better for me to have a diff. So, here one: mp16@c41fa14

I have added a "else" in order to not unlock twice the mutex. Is it ok ?

[lminiero]

No, that's not correct. I'll prepare a PR.

[mp16]

Sorry, I misread it. I think I get it this time: https://github.com/meetecho/janus-gateway/compare/master...mp16:fix_1982?expand=1

[mp16]

Thanks.

[lminiero]

I created a PR with the fix, so please test that to see if it works as expected for you.

[mp16]

I will, thanks.

[mivuDing]

@mp16 Maybe, I think the same fix should be done in janus_videoroom_setup_media and janus_videoroom_hangup_media, please see the follow attach. I hope it can works for you.
videoroom_update.zip

@lminiero Can you help me to check the diff patch of the attach? Is it OK? Thanks very much!

[lminiero]

@mivuDing sorry but I won't open a zip file. Please share a diff, or open a pull request with the proposed changes.

[mp16]

Deadlock again this morning https://pastebin.com/EJK6Fas9 after like 12 hours of execution.

Stuck again in janus_videoroom_hangup_media, janus_videoroom_slow_link and janus_videoroom_hangup_subscriber.

@mivuDing I agree, I think there is deadlock in others functions too. I have created a pull request by using your zip file. I will try your fix after the review by @lminiero

[mivuDing]

@mp16 Thanks for the PR. If the fix work for you or didn't work, please let me know.
@lminiero Please help to review the PR, which mp16 had been pushed. Thanks a lot!

[lminiero]

@mivuDing as @atoppi said, this looks fine, but we'll need some more testing before we can merge. About that, please sign our CLA, since even if someone else created the PR for you, the contribution was actually yours, and there are credits to you in the commit.

[mivuDing]

@lminiero I have signed the CLA, thanks very much!