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

Decrease refcount for subscriber #3493

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

m08pvv
Copy link
Contributor

@m08pvv m08pvv commented Dec 18, 2024

Noticed a suspiciously missing janus_refcount_decrease which could be the cause of this leak

@atoppi
Copy link
Member

atoppi commented Dec 18, 2024

Good catch! That unref is missing for sure, so this patch LGTM.

It's worth mentioning there is no strict evidence in the group discussion that can correlate the observed leak with the slow-link events. The refcount dump just mentions many calls to janus_videoroom_session_get_subscriber.

	Line 171673: [plugins/janus_videoroom.c:janus_videoroom_handler:10313:init] 0x60c000502130 (1)
	Line 171674: [plugins/janus_videoroom.c:janus_videoroom_handler:10314:increase] 0x60c000502130 (2)
	Line 172158: [plugins/janus_videoroom.c:janus_videoroom_handler:10650:decrease] 0x60c000502130 (1)
	Line 172270: [plugins/janus_videoroom.c:janus_videoroom_session_get_subscriber:4261:increase] 0x60c000502130 (2)
	Line 172333: [plugins/janus_videoroom.c:janus_videoroom_handler:12633:decrease] 0x60c000502130 (1)
	Line 172380: [plugins/janus_videoroom.c:janus_videoroom_session_get_subscriber:4261:increase] 0x60c000502130 (2)
	Line 172383: [plugins/janus_videoroom.c:janus_videoroom_setup_media:8634:decrease] 0x60c000502130 (1)
	Line 174357: [plugins/janus_videoroom.c:janus_videoroom_session_get_subscriber:4261:increase] 0x60c000502130 (2)
	Line 174502: [plugins/janus_videoroom.c:janus_videoroom_session_get_subscriber:4261:increase] 0x60c000502130 (3)
	Line 178481: [plugins/janus_videoroom.c:janus_videoroom_session_get_subscriber:4261:increase] 0x60c000502130 (4)
	Line 178501: [plugins/janus_videoroom.c:janus_videoroom_hangup_media_internal:9460:decrease] 0x60c000502130 (3)
	Line 181830: [plugins/janus_videoroom.c:janus_videoroom_session_get_subscriber:4261:increase] 0x60c000502130 (4)
	Line 181833: [plugins/janus_videoroom.c:janus_videoroom_hangup_media_internal:9460:decrease] 0x60c000502130 (3)
	Line 181836: [plugins/janus_videoroom.c:janus_videoroom_destroy_session:4534:increase] 0x60c000502130 (4)
	Line 181839: [plugins/janus_videoroom.c:janus_videoroom_subscriber_destroy:2688:decrease] 0x60c000502130 (3)
	Line 181841: [plugins/janus_videoroom.c:janus_videoroom_destroy_session:4553:decrease] 0x60c000502130 (2)

Nevertheless slow-links could explain that leak and the patch looks good.
Thanks @m08pvv.

@m08pvv
Copy link
Contributor Author

m08pvv commented Dec 18, 2024

It's worth mentioning there is no strict evidence in the group discussion that can correlate the observed leak with the slow-link events. The refcount dump just mentions many calls to janus_videoroom_session_get_subscriber.

janus_videoroom_slow_link calls janus_videoroom_session_get_subscriber which increments the refcount and the logger logs it as janus_videoroom_session_get_subscriber because the refcount increases from it, so it's one of that calls

@atoppi
Copy link
Member

atoppi commented Dec 18, 2024

It's worth mentioning there is no strict evidence in the group discussion that can correlate the observed leak with the slow-link events. The refcount dump just mentions many calls to janus_videoroom_session_get_subscriber.

janus_videoroom_slow_link calls janus_videoroom_session_get_subscriber which increments the refcount and the logger logs it as janus_videoroom_session_get_subscriber because the refcount increases from it, so it's one of that calls

Yes I know how refcount debugging works. I just meant there are several potential entry points for janus_videoroom_session_get_subscriber in the plugin.
Anyway I've just double checked the code and it seems the only place where the unref is missing is the spot you already discovered.
Thanks again.

@lminiero
Copy link
Member

I agree that's the likely culprit. Let me check if it's there in 0.x too and then I'll merge (and in case backport).

@lminiero
Copy link
Member

It's missing there too, so I'll cherry-pick the commit after the merge. Thanks!

@lminiero lminiero merged commit 4fc066f into meetecho:master Dec 18, 2024
8 checks passed
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.

3 participants