[core] Fixed proper reporting of sending blocked state. Fixed API value for connect-non-blocking #1698

ethouris · 2020-12-07T09:57:34Z

Collecting information about blocked sending (with none succeeded)
In non-blocking mode, this should result in reporting AGAIN error
In blocking mode, it should block until at least one socket is ready for sending, and then only report errors if none succeeded.

Additionally: the value returned in case of blocking-mode srt_connect_group it should return the socket ID of the first successful socket, while in case of non-blocking mode it always returns 0.

Findings (update):

[core] Fixed bug: tsbpd might miss m_bClosing flag setting, flag not always properly set. #1709 seems to be prematurely merged
Changes in the refactoring in the group.cpp file could have brought in some of the fixes (counting successful and blocked sockets)

NOT updating, turning to draft.

…ue for connect-non-blocking

… exit checks in tsbpd. Suppressed IPE log when unsubscribing

maxsharabayko · 2020-12-08T11:26:34Z

srtcore/core.cpp

        }

        HLOGC(tslog.Debug, log << self->CONID() << "tsbpd: WAKE UP!!!");
    }
+ExitLoops:


No new goto statements, please.

I need to exit two loops, and additionally maintain the thread checker.

maxsharabayko · 2020-12-08T11:27:50Z

srtcore/core.cpp

+            bool signaled = false;
+            while (!signaled)
+            {
+                // For safety reasons, do wakeup once per 1s and re-check the flag.
+                THREAD_PAUSED();
+                signaled = tsbpd_cc.wait_for(seconds_from(1));
+                THREAD_RESUMED();
+                if (self->m_bClosing)
+                {
+                    HLOGC(tslog.Debug, log << "tsbpd: IPE? Closing flag set in the meantime of waiting. Exiting");
+                    goto ExitLoops;
+                }
+            }


Why not just let TSBPD to re-check the receiver buffer even in case of a spurious wake-up?

Actually, yes, might be a better idea. OTOH this 1s delay is too much if that deadlock would have happened when closing a socket in the group receiver.

…tsbpd_cc wait 125ms interrupt for sanity check

srtcore/group.cpp

maxsharabayko · 2020-12-08T16:52:53Z

srtcore/group.cpp

+            int swait_timeout = 0;
+
+            // There's also a hidden condition here that is the upper if condition.
+            is_pending_blocked = (nsuccessful == 0);
+
+            // If this is the case when 
+            if (m_bSynSending && is_pending_blocked)
+            {
+                HLOGC(gslog.Debug, log << "grp/sendBroadcast: will block for " << m_iSndTimeOut << " - waiting for any writable in blocking mode");
+                swait_timeout = m_iSndTimeOut;
+            }


Code duplication: the same was used in send_broadcast... Consider some common funtion.

I will. There's a PR for it, but we need to be clear off with all potential problems to solve first. There are marked places that should make this process easy enough. Note however that the things happen differently in broadcast and backup, which means that even conditions for getting sockets blocked are not exactly the same.

maxsharabayko · 2020-12-08T16:56:05Z

srtcore/core.cpp

 void CUDT::updateBrokenConnection()
 {
+    HLOGC(smlog.Debug, log << "updateBrokenConnection: setting closing=true and taking out epoll events");
+    m_bClosing = true;


Does this fix require other changes introduced in the PR?

This is simply a completion for all cases when releaseSync is called, which is connected to the fact that releaseSync is about to make TSBPD thread exit, and it can only exit if m_bClosing == true.

Does only this change make sense without the rest changes in this PR?

Rest of the changes are formal fixes for real problems, others - including this one - are "sanity fixes". This flag must be set true when the TSBPD thread is expected to exit otherwise it risks a deadlock. Relying on that a flag is set appropriately before exitting isn't the best approach, but it's simply derived from UDT and it's a consequence of how the TSBPD thread is expected to work. Changes here were due to an in-between lifted lock on m_RecvLock, which was newly introduced with the groups.

Let's move this fix to a separate PR, so it is not blocked by the review of other changes introduced here.

Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>

maxsharabayko · 2020-12-14T17:13:26Z

srtcore/core.cpp

@@ -7819,15 +7854,20 @@ void CUDT::destroySynch()
 void CUDT::releaseSynch()
 {
    SRT_ASSERT(m_bClosing);
+    if (!m_bClosing)
+    {
+        HLOGC(smlog.Debug, log << "releaseSynch: IPE: m_bClosing not set to false, TSBPD might hangup!");


This might rather be an error message: LOGC(smlog.Error, ...).

maxsharabayko · 2020-12-15T09:57:43Z

srtcore/api.cpp

+    // Return value rules:
+    // In non-blocking mode:
+    // - return Socket ID, if:
+    //   - you requested only one connection in this call
+    // In blocking mode:
+    // - return Socket ID, if:
+    //   - you requested only one connection in this call
+    //   - you connect a group that was not connected yet
+    // - otherwise return 0
+


"if you requested only one connection in this call"
Not clear. I guess the meaning is that there is only one target passed to this group connect function (arraysize).

This means that it's so if you do call srt_connect or if you do srt_connect_group with an array of size 1.

maxsharabayko · 2020-12-15T10:01:04Z

srtcore/api.cpp

+    if (arraysize > 1)
+        retval = 0;


Given that arraysize is 400 lines above from this line, it would make sense to make it const, so that no one mistakenly changes it in between.

int CUDTUnited::groupConnect(CUDTGroup* pg, SRT_SOCKGROUPCONFIG* targets, const int arraysize

maxsharabayko · 2020-12-15T10:07:25Z

srtcore/core.cpp

+            {
+                // For safety reasons, do wakeup once per 1/8s and re-check the flag.
+                // This should be enough long time that during a normal transmission
+                // the TSBPD thread would be woken up much earlier when required by
+                // ACK per ACK timer (at most 10ms since the last check) and in case
+                // when this might result in a deadlock, it would only hold up to 125ms,
+                // which should be little harmful for the application. NOTE THAT THIS
+                // IS A SANITY CHECK FOR A SITUATION THAT SHALL NEVER HAPPEN.
+                THREAD_PAUSED();
+                signaled = tsbpd_cc.wait_for(milliseconds_from(125));


It will hide possible synchronization issues, and it would much harder to track them.
Also might introduce a potential minor performance degradation.

CUDT::releaseSynch() notifies m_RcvTsbPdCond used in this tsbpd_cc. If it happens to be blocked forever, then there is another issue to fix.

That's only a sanity check, and I doubt the performance degradation here, although might be that 250ms would be a less likely watchdog to bite, while applications also usually have their own independent watchdogs. Initially I planned 1s here, but that would be way too much. I understand that it might make errors kinda hidden, but OTOH a deadlock here would make a scrap in video transmission (so it won't go "invisible", just it won't make a total disaster).

Note also that releaseSynch won't be called another time, that's true, but TSBPD should simply exit on seeing m_bClosing flag anyway.

[core] Fixed proper reporting of sending blocked state. Fixed API val…

4a95481

…ue for connect-non-blocking

ethouris changed the title ~~[core] Fixed proper reporting of sending blocked state. Fixed API val…ue for connect-non-blocking~~ [core] Fixed proper reporting of sending blocked state. Fixed API value for connect-non-blocking Dec 7, 2020

ethouris marked this pull request as draft December 7, 2020 14:26

Fixed return 0 on multi-connect call. Added lock-preventive emergency…

c272a05

… exit checks in tsbpd. Suppressed IPE log when unsubscribing

maxsharabayko reviewed Dec 8, 2020

View reviewed changes

Mikołaj Małecki added 2 commits December 8, 2020 14:44

Fixed TSBPD deadlock-prone continuation. Demoted epoll IPE log. Made …

834b702

…tsbpd_cc wait 125ms interrupt for sanity check

Fixed return value from srt_connect for groups

8b4d0b7

ethouris marked this pull request as ready for review December 8, 2020 16:43

Merge branch 'master' into dev-fix-group-ut-findings

f0d185c

maxsharabayko reviewed Dec 8, 2020

View reviewed changes

ethouris and others added 2 commits December 8, 2020 18:06

Apply suggestions from code review

575807b

Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>

Fixed test that reused a dead socket

fa55144

This comment has been minimized.

Sign in to view

maxsharabayko added [core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior labels Dec 10, 2020

maxsharabayko added this to the v1.4.3 milestone Dec 10, 2020

maxsharabayko reviewed Dec 14, 2020

View reviewed changes

maxsharabayko reviewed Dec 15, 2020

View reviewed changes

Mikołaj Małecki added 3 commits December 17, 2020 17:05

Merged. Upgraded IPE log to Error

4b74a4c

Updated constness of arraysize

ece9a28

Updated to latest upstream

5572363

maxsharabayko modified the milestones: v1.4.3, v1.4.4 Apr 13, 2021

maxsharabayko modified the milestones: v1.4.4, Backlog Aug 18, 2021

ethouris marked this pull request as draft September 24, 2024 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Fixed proper reporting of sending blocked state. Fixed API value for connect-non-blocking #1698

[core] Fixed proper reporting of sending blocked state. Fixed API value for connect-non-blocking #1698

ethouris commented Dec 7, 2020 •

edited

Loading

maxsharabayko Dec 8, 2020

ethouris Dec 8, 2020

maxsharabayko Dec 8, 2020

ethouris Dec 8, 2020

maxsharabayko Dec 8, 2020

ethouris Dec 8, 2020

maxsharabayko Dec 8, 2020

ethouris Dec 8, 2020

maxsharabayko Dec 10, 2020

ethouris Dec 14, 2020

maxsharabayko Dec 14, 2020

This comment has been minimized.

maxsharabayko Dec 14, 2020

maxsharabayko Dec 15, 2020

ethouris Dec 17, 2020

maxsharabayko Dec 15, 2020

maxsharabayko Dec 15, 2020

ethouris Dec 17, 2020 •

edited

Loading

[core] Fixed proper reporting of sending blocked state. Fixed API value for connect-non-blocking #1698

Are you sure you want to change the base?

[core] Fixed proper reporting of sending blocked state. Fixed API value for connect-non-blocking #1698

Conversation

ethouris commented Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethouris Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

ethouris commented Dec 7, 2020 •

edited

Loading

ethouris Dec 17, 2020 •

edited

Loading