fix moutpoint_mutex deadlock when rtsp reconnect fail

[lucylu-star]

fix moutpoint_mutex deadlock when rtsp reconnect fail.

[januscla]

Thanks for your contribution, @lucylu-star! Please make sure you sign our CLA, as it's a required step before we can merge this.

[lminiero]

Thanks for the contribution, but this doesn't seem to work. Please see my notes below.

[lminiero]

Added some more notes. Thanks!

[lucylu-star]

del newline and brackets

[lucylu-star]

hi lminiero，
I have adjust something and add comments.
the push request is ok or not now?
Should I adjust others?

[lminiero]

Added just one note inline. Thanks!

[lucylu-star]

yes,you are right. Thanks. I have done that and push request again. (remove the first check and only keep the first check) Please help to review it again. Is it ok now?

[lminiero]

I found out why we had made those changes. You can find the rationale in these two commits:

ced1c7a
262e997

The first commit started using mountpoints_mutex to avoid collisions on local ports, while the second limited the mutex usage only around janus_streaming_rtsp_parse_sdp, which is indeed the method that may then allocate new local ports. We use a slider for ports, and so need a global mutex to avoid different mountpoints taking the same port.

For this reason, we cannot accept changing the mutex from mountpoints_mutex to mp->mutex as you did. I think adding the checks on mp->destroyed as you did is correct, instead, so if you update your patch to only do that it can definitely be merged.

[lminiero]

@lucylu-star ping 🙂
Please notice we just merged some changes to the reconnect code, which you can see in #2598

[eLvErDe]

Hello,

@lucylu-star I guess you just fixed a 3-years old bug occuring in production on a bi-monthly basis.
I'll provide more feedbacks tomorrow but it looks like I cannot reproduce anymore.

@lucylu-star got a paypal account or something ? Would like to send a small tip for that :)

Regards

[eLvErDe]

@lminiero confirmed fixing janus deadlock for us, please consider merging it or rewrite yourself if the submitter is gone ;-)

Regards

[lminiero]

As I already said, it can't be merged as it is. If you can verify that limiting the patch to what I mentioned still fixes your issue, I can do a separate commit.

[eLvErDe]

As I already said, it can't be merged as it is. If you can verify that limiting the patch to what I mentioned still fixes your issue, I can do a separate commit.

I sure can, can you show me which line must be changed ? I'll try to get you a feedback today

[lminiero]

It's explained in this comment I added a few weeks ago:

For this reason, we cannot accept changing the mutex from mountpoints_mutex to mp->mutex as you did. I think adding the checks on mp->destroyed as you did is correct, instead, so if you update your patch to only do that it can definitely be merged.

[eLvErDe]

Okay I guess I got it, will keep you informed asap

[lucylu-star]

Hello, I think the push request can fix the deadlock issue. I have merge and commit it today again. Is it ok now? Besides, got paypal account and send a small tip for me , really？ Thanks. : ) At 2021-04-04 05:36:45, "Adam Cécile" ***@***.***> wrote: Hello, @lucylu-star I guess you just fixed a 3-years old bug occuring in production on a bi-monthly basis. I'll provide more feedbacks tomorrow but it looks like I cannot reproduce anymore. @lucylu-star got a paypal account or something ? Would like to send a small tip for that :) Regards — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[lminiero]

@lucylu-star mh it looks like your merge did something weird, as I now see part of other commits (e.g., the RTSP reconnect PR) in your commits. Maybe this needs a rebase on master and a force push?

[lucylu-star]

Do you mean the merge commit which have other people's commit is unnecessary? If so, I will push request only have my fix code again. ok? or any suggestions? At 2021-04-07 22:34:58, "Lorenzo Miniero" ***@***.***> wrote: @lucylu-star mh it looks like your merge did something weird, as I now see part of other commits (e.g., the RTSP reconnect PR) in your commits. Maybe this needs a rebase on master and a force push? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[eLvErDe]

Yes you need your fix only, you need to force push your branch with --push after rebasing against current master.

And yes I'm serious about paypal, send me your email at [email protected]

[lminiero]

Still the same issues with commits 🙁
Github reports conflicts too.

[eLvErDe]

@lucylu-star not a git expert right ? :p

If you want you can add me to your GitHub fork of janus, I'll fix it for you

[lucylu-star]

I have done as follows, git remote add upstream https://github.com/meetecho/janus-gateway.git git fetch upstream git merge upstream/master git push origin master but, failed as you can see. I don't how to do it. What should the git command list are ? At 2021-04-08 20:14:12, "Adam Cécile" ***@***.***> wrote: @lucylu-star not a git expert right ? :p If you want you can add me to your GitHub fork of janus, I'll fix it for you — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[lminiero]

You need to rebase, not merge. That said, it might be easier to create a new PR from scratch.

[lucylu-star]

ok, I create a new PR. can I close the old PR, or you close it? At 2021-04-08 21:24:00, "Lorenzo Miniero" ***@***.***> wrote: You need to rebase, not merge. That said, it might be easier to create a new PR from scratch. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[lminiero]

Now I see a rebase but the commits removed those new features instead 🤭
Please just create a new PR, it will be easier that way: I can close this one, no problem.

[lminiero]

Thanks, looks good now! @eLvErDe could you give this patch a test?

[eLvErDe]

@lminiero kinda busy right now but I'll find some time

[eLvErDe]

@lminiero built and deployed, I asked someone else to perform the load test and will get back to you

[eLvErDe]

@lminiero confirmed, no crash after intensive testing

[lminiero]

Thanks for the feedback, and thanks again to @lucylu-star for the contribution and patience: merging this then!

[eLvErDe]

well, I just got this really weird behavior:

20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1134624841, index 0)
20:31:36 janus janus[16443]: [100290576] New video stream! (ssrc=1694606358, index 0)

Do you think it can be related ?

[eLvErDe]

Just rolled back to 0.10.10 and I'm getting the same log so I guess it's related to something else

Oh god, what a day :/

[lminiero]

I doubt it, this PR only interrupts any RTSP reconnect attempt if the mountpoint has been destroyed.

[eLvErDe]

Yes, just tested with previous version, got the same bug. Server shutdown for now others seems to be working correctly.
Any idea what it could be ?

[lminiero]

No, and at any rate it shouldn't be discussed here. If you think it's a bug and not something you're doing wrong, please open a new issue.

[eLvErDe]

Well, I thought it could have been related. Nevermind, I have no idea why this crap happened and no way to reproduce so let's just forget about this