Skip to content

[MAINT] Added setting CLOSING state when closing a socket #2643

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 8, 2023
Merged

[MAINT] Added setting CLOSING state when closing a socket #2643

merged 4 commits into from
Feb 8, 2023

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Feb 3, 2023

  1. setting SRTS_CLOSING state on a socket that was requested to be closed.
  2. Added time tracking when closing a socket with tracked wiping to track the time taken for wiping out a socket.

@ethouris ethouris added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Feb 3, 2023
@ethouris ethouris added this to the v1.5.2 milestone Feb 3, 2023
maxsharabayko
maxsharabayko reviewed Feb 6, 2023
View reviewed changes
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

Setting the state to SRTS_CLOSING reduces the time to actually free the listener socket from 4.5 s to 1.5 s.
Adding CSync::notify_one_relaxed(m_GCStopCond); at the end of CUDTUnited::close() further reduces the time to 1 second by notifying the GC thread to run a re-check.

@maxsharabayko
Copy link
Collaborator

Git Diff

(For my reference).

diff --git a/srtcore/api.cpp b/srtcore/api.cpp
index f052c96..955e535 100644
--- a/srtcore/api.cpp
+++ b/srtcore/api.cpp
@@ -1974,7 +1974,10 @@ int srt::CUDTUnited::close(CUDTSocket* s)
     if (s->m_Status == SRTS_LISTENING)
     {
         if (s->core().m_bBroken)
+        {
+            LOGC(smlog.Note, log << s->core().CONID() << "CLOSING (broken but listening)");
             return 0;
+        }

         s->m_tsClosureTimeStamp = steady_clock::now();
         s->core().m_bBroken     = true;
@@ -1988,8 +1991,9 @@ int srt::CUDTUnited::close(CUDTSocket* s)
         // be unable to bind to this port that the about-to-delete listener
         // is currently occupying (due to blocked slot in the RcvQueue).

-        HLOGC(smlog.Debug, log << s->core().CONID() << "CLOSING (removing listener immediately)");
+        LOGC(smlog.Note, log << s->core().CONID() << "CLOSING (removing listener immediately)");
         s->core().notListening();
+        s->m_Status = SRTS_CLOSING;

         // broadcast all "accept" waiting
         CSync::lock_notify_all(s->m_AcceptCond, s->m_AcceptLock);
@@ -2004,7 +2008,7 @@ int srt::CUDTUnited::close(CUDTSocket* s)
         s->core().closeInternal();

         // synchronize with garbage collection.
-        HLOGC(smlog.Debug,
+        LOGC(smlog.Note,
               log << "@" << u << "U::close done. GLOBAL CLOSE: " << s->core().CONID()
                   << "Acquiring GLOBAL control lock");
         ScopedLock manager_cg(m_GlobControlLock);
@@ -2119,6 +2123,8 @@ int srt::CUDTUnited::close(CUDTSocket* s)
     }
     */

+    CSync::notify_one_relaxed(m_GCStopCond);
+
     return 0;
 }

@@ -2602,6 +2608,7 @@ void srt::CUDTUnited::checkBrokenSockets()

         if (s->m_Status == SRTS_LISTENING)
         {
+            LOGC(smlog.Note, log << "checkBrokenSockets: @" << s->m_SocketID << " status LISTENING.");
             const steady_clock::duration elapsed = steady_clock::now() - s->m_tsClosureTimeStamp;
             // A listening socket should wait an extra 3 seconds
             // in case a client is connecting.
@@ -2616,6 +2623,7 @@ void srt::CUDTUnited::checkBrokenSockets()
         // available data is "ready to play").
                  && s->core().m_pRcvBuffer->hasAvailablePackets())
         {
+            LOGC(smlog.Note, log << "checkBrokenSockets: @" << s->m_SocketID << " m_pRcvBuffer has packets.");
             const int bc = s->core().m_iBrokenCounter.load();
             if (bc > 0)
             {
@@ -2634,7 +2642,7 @@ void srt::CUDTUnited::checkBrokenSockets()
         }
 #endif

-        HLOGC(smlog.Debug, log << "checkBrokenSockets: moving BROKEN socket to CLOSED: @" << i->first);
+        LOGC(smlog.Note, log << "checkBrokenSockets: moving BROKEN socket to CLOSED: @" << i->first);

         // close broken connections and start removal timer
         s->setClosed();

Unit Test

(No need to include it in the PR. Just for my reference).

#include <gtest/gtest.h>
#include <thread>
#include <chrono>
#include <string>
#include <map>

#ifdef _WIN32
#define INC_SRT_WIN_WINTIME // exclude gettimeofday from srt headers
#endif

#include "srt.h"

TEST(Listener, Restart)
{
    srt_startup();
    srt_setloglevel(LOG_NOTICE);

    auto s = srt_create_socket();

    sockaddr_in bind_sa;
    memset(&bind_sa, 0, sizeof bind_sa);
    bind_sa.sin_family = AF_INET;
    ASSERT_EQ(inet_pton(AF_INET, "127.0.0.1", &bind_sa.sin_addr), 1);
    bind_sa.sin_port = htons(5555);

    EXPECT_NE(srt_bind(s, (sockaddr*)&bind_sa, sizeof bind_sa), -1);
    EXPECT_NE(srt_listen(s, 5), -1);

    std::this_thread::sleep_for(std::chrono::milliseconds(500));

    srt_close(s);
    
    s = srt_create_socket();
    int optval = 100;
    int optlen = sizeof optval;
    EXPECT_NE(srt_setsockflag(s, SRTO_IPTTL, (void*)&optval, optlen), SRT_ERROR) << srt_getlasterror_str();

    const auto time_start = std::chrono::steady_clock::now();
    while (srt_bind(s, (sockaddr*)&bind_sa, sizeof bind_sa) == -1)
    {
        std::this_thread::sleep_for(std::chrono::milliseconds(500));
    }

    std::cerr << "Binding took " << std::chrono::duration_cast<std::chrono::milliseconds>((std::chrono::steady_clock::now() - time_start)).count() << '\n';

    //EXPECT_NE(srt_bind(s, (sockaddr*)&bind_sa, sizeof bind_sa), -1);
    EXPECT_NE(srt_listen(s, 5), -1);

    srt_close(s);
    srt_cleanup();
}

@maxsharabayko maxsharabayko merged commit 65bef37 into Haivision:master Feb 8, 2023
@maxsharabayko maxsharabayko mentioned this pull request Dec 4, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxsharabayko maxsharabayko maxsharabayko left review comments

Assignees
No one assigned
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Milestone
v1.5.2
Development

Successfully merging this pull request may close these issues.
2 participants
@ethouris @maxsharabayko