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 missing increment of the reference count on every call to startup() #3100

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

Conversation

mbechard
Copy link

@mbechard mbechard commented Jan 3, 2025

also change how the implicit startup is tracked.
The extra change is needed since the startup() calls in
srt::CUDT::socket()
and
srt::CUDT::createGroup
would endlessly increment the startup counter as sockets were created.

This also adds handling for the case where the user code fails to call
srt_startup when initially using the library, but does later on.
It better matches up the implicit startup() with an implicit cleanup().

fixes #3098

@mbechard mbechard marked this pull request as draft January 3, 2025 22:08
@ethouris
Copy link
Collaborator

ethouris commented Jan 6, 2025

As for me it looks ok, but Max will be back in the second half of January.

@ethouris ethouris added this to the v1.5.5 milestone Jan 6, 2025
@mbechard
Copy link
Author

mbechard commented Jan 6, 2025

Still has bugs with existing code, as per the issue discussion

also change how the implicit startup is tracked.
The extra change is needed since the startup() calls in
srt::CUDT::socket()
and
srt::CUDT::createGroup
would endlessly increment the startup counter as sockets were created.

This also adds handling for the case where the user code fails to call
srt_startup when initially using the library, but does later on.
It better matches up the implicit startup() with an implicit cleanup().

fixes Haivision#3098
@mbechard mbechard marked this pull request as ready for review January 6, 2025 21:18
@mbechard
Copy link
Author

mbechard commented Jan 6, 2025

I think this modified solution is reasonable to handle the remaining issues.

return 1;
return;
m_bImplicitStartup = true;
startup();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double locking on m_InitLock here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh is the lock primitive used in this library not recursive?

@ethouris
Copy link
Collaborator

ethouris commented Jan 8, 2025

Wait, there's one thing I don't understand.

Why do you need to prevent the call to cleanup()? The call to cleanup() should be able to be done multiple times, even if at least one of the series has done the real cleanup.

The only rule was that there should be done as many (meaning, the minimum) cleanup calls as there was startup calls, but extra cleanup() calls, which actually do nothing, should be allowed. Have you found a problem with extra cleanups?

@mbechard
Copy link
Author

mbechard commented Jan 8, 2025

This doesn't disallow extra cleanup in general, that allowance remains in place. The change here though is that it avoids an unpaired cleanup from internal code. Doing a cleanup internally when a startup wasn't done internally can hide bugs in the API users code. And since the cleanup done in srt::CUDTUnited::~CUDTUnited() is only just doing a single count decrement, whether or not it actually avoids the app closing without doing the cleanup isn't dependable.
If anything, I would contend that srt::CUDTUnited::~CUDTUnited() should force a cleanup no matter what counter is, as long as it's > 0. That would ensure the cleanup on shutdown.

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.

[BUG] Regression with commit bc2f48e057644ab9
2 participants