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 instance count on every call to startup() #3100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions srtcore/api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,13 +198,8 @@

srt::CUDTUnited::~CUDTUnited()
{
// Call it if it wasn't called already.
// This will happen at the end of main() of the application,
// when the user didn't call srt_cleanup().
if (m_bGCStatus)
{
cleanup();
}
// force cleanup, even if the instance count is > 1
cleanup(true);

releaseMutex(m_GlobControlLock);
releaseMutex(m_IDLock);
Expand Down Expand Up @@ -233,13 +228,20 @@
return os.str();
}

int srt::CUDTUnited::startup()
int srt::CUDTUnited::startup(bool implicit)
{
ScopedLock gcinit(m_InitLock);
if (m_bGCStatus)

if (implicit && m_iInstanceCount > 0)
ethouris marked this conversation as resolved.
Show resolved Hide resolved
// [reinstate in 1.6.0] return m_iInstanceCount;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
return 1;

if (m_iInstanceCount++ > 0)
// [reinstate in 1.6.0] return m_iInstanceCount;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
return 1;

if (m_bGCStatus)

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
// [reinstate in 1.6.0] return m_iInstanceCount;
return 1;

// Global initialization code
Expand Down Expand Up @@ -268,7 +270,7 @@
return 0;
}

int srt::CUDTUnited::cleanup()
int srt::CUDTUnited::cleanup(bool force)
{
// IMPORTANT!!!
// In this function there must be NO LOGGING AT ALL. This function may
Expand All @@ -280,9 +282,14 @@
// executing this procedure.
ScopedLock gcinit(m_InitLock);

if (--m_iInstanceCount > 0)
return 0;
if (!force)
{
if (m_iInstanceCount == 0 || --m_iInstanceCount > 0)

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
// [reinstate in 1.6.0] return m_iInstanceCount;
return 0;
}

// Cleanup done before already
if (!m_bGCStatus)
return 0;

Expand All @@ -298,13 +305,16 @@
CSync::notify_one_relaxed(m_GCStopCond);
m_GCThread.join();

if (force)
m_iInstanceCount = 0;
m_bGCStatus = false;

// Global destruction code
#ifdef _WIN32
WSACleanup();
#endif

// Cleanup done at all.
return 0;
}

Expand Down Expand Up @@ -3469,7 +3479,7 @@

SRTSOCKET srt::CUDT::socket()
{
uglobal().startup();
uglobal().startup(true);

try
{
Expand Down Expand Up @@ -3519,7 +3529,7 @@
SRTSOCKET srt::CUDT::createGroup(SRT_GROUP_TYPE gt)
{
// Doing the same lazy-startup as with srt_create_socket()
uglobal().startup();
uglobal().startup(true);

try
{
Expand Down
15 changes: 13 additions & 2 deletions srtcore/api.h
Original file line number Diff line number Diff line change
Expand Up @@ -257,12 +257,14 @@ class CUDTUnited
static std::string CONID(SRTSOCKET sock);

/// initialize the UDT library.
/// @param [in] implicit should be set to true for internal implicit startup() calls.
/// @return 0 if success, otherwise -1 is returned.
int startup();
int startup(bool implicit = false);

/// release the UDT library.
/// @param [in] force cleanup regardless of the instance count.
/// @return 0 if success, otherwise -1 is returned.
int cleanup();
int cleanup(bool force = false);

/// Create a new UDT socket.
/// @param [out] pps Variable (optional) to which the new socket will be written, if succeeded
Expand Down Expand Up @@ -542,6 +544,15 @@ class CUDTUnited
int m_iInstanceCount; // number of startup() called by application
SRT_ATTR_GUARDED_BY(m_InitLock)
bool m_bGCStatus; // if the GC thread is working (true)
public:

std::pair<int, bool> getInstanceStatus()
{
sync::ScopedLock lk(m_InitLock);
return std::make_pair(m_iInstanceCount, m_bGCStatus);
}

private:

SRT_ATTR_GUARDED_BY(m_InitLock)
sync::CThread m_GCThread;
Expand Down
6 changes: 5 additions & 1 deletion test/test_bonding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,6 @@ TEST(Bonding, ConnectNonBlocking)
EXPECT_NE(srt_bind(g_listen_socket, (sockaddr*)&bind_sa, sizeof bind_sa), -1);
const int yes = 1;
srt_setsockflag(g_listen_socket, SRTO_GROUPCONNECT, &yes, sizeof yes);
EXPECT_NE(srt_listen(g_listen_socket, 5), -1);

int lsn_eid = srt_epoll_create();
int lsn_events = SRT_EPOLL_IN | SRT_EPOLL_ERR | SRT_EPOLL_UPDATE;
Expand Down Expand Up @@ -761,6 +760,11 @@ TEST(Bonding, ConnectNonBlocking)

cout << "[A] Waiting for accept\n";

// Yield to allow the "too early" sending to fail
std::this_thread::yield();

EXPECT_NE(srt_listen(g_listen_socket, 5), -1);

// This can wait in infinity; worst case it will be killed in process.
int uwait_res = srt_epoll_uwait(lsn_eid, ev, 3, -1);
EXPECT_EQ(uwait_res, 1);
Expand Down
37 changes: 37 additions & 0 deletions test/test_common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,44 @@
#include "test_env.h"
#include "utilities.h"
#include "common.h"
#include "api.h"

using namespace srt;

TEST(General, Startup)
{
// Should return 0 if it was run for the first time
// and actually performed the initialization.
EXPECT_EQ(srt_startup(), 0);

EXPECT_EQ(srt::CUDT::uglobal().getInstanceStatus(), std::make_pair(1, true));

// Every next one should return the number of nested instances
// [reinstate in 1.6.0] EXPECT_EQ(srt_startup(), 2);
srt_startup();
EXPECT_EQ(srt::CUDT::uglobal().getInstanceStatus(), std::make_pair(2, true));

// Now let's pair the first instance, should NOT execute
// [reinstate in 1.6.0] EXPECT_EQ(srt_cleanup(), 1);
srt_cleanup();

EXPECT_EQ(srt_cleanup(), 0);

// Second cleanup, should report successful cleanup even if nothing is done.
EXPECT_EQ(srt_cleanup(), 0);

// Now let's start with getting the number of instances
// from implicitly created ones.
SRTSOCKET sock = srt_create_socket();

EXPECT_EQ(srt::CUDT::uglobal().getInstanceStatus(), std::make_pair(1, true));

EXPECT_EQ(srt_close(sock), 0);

// Do the cleanup again, to not leave it up to the global destructor.
EXPECT_EQ(srt_cleanup(), 0);
}

void test_cipaddress_pton(const char* peer_ip, int family, const uint32_t (&ip)[4])
{
const int port = 4200;
Expand Down Expand Up @@ -44,6 +79,8 @@ void test_cipaddress_pton(const char* peer_ip, int family, const uint32_t (&ip)[
// Example IPv4 address: 192.168.0.1
TEST(CIPAddress, IPv4_pton)
{
// Check the NEXT TEST of General/Startup, if it has done the cleanup.
EXPECT_EQ(srt::CUDT::uglobal().getInstanceStatus(), std::make_pair(0, false));
srt::TestInit srtinit;
const char* peer_ip = "192.168.0.1";
const uint32_t ip[4] = {htobe32(0xC0A80001), 0, 0, 0};
Expand Down
4 changes: 1 addition & 3 deletions test/test_connection_timeout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ TEST(TestConnectionAPI, Accept)
using namespace std::chrono;
using namespace srt;

srt_startup();
TestInit srtinit;

const SRTSOCKET caller_sock = srt_create_socket();
const SRTSOCKET listener_sock = srt_create_socket();
Expand Down Expand Up @@ -266,8 +266,6 @@ TEST(TestConnectionAPI, Accept)
}
srt_close(caller_sock);
srt_close(listener_sock);

srt_cleanup();
}


Loading