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

Fixed error reporting on connect/accept. #1388

Merged
merged 7 commits into from
Jul 30, 2020

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Jun 30, 2020

Fixes #1242

@ethouris ethouris self-assigned this Jun 30, 2020
@ethouris ethouris added [core] Area: Changes in SRT library core [docs] Area: Improvements or additions to documentation Impact: Significant Type: Maintenance Work required to maintain or clean up the code labels Jun 30, 2020
@ethouris ethouris added this to the v1.5.0 - Sprint 18 milestone Jun 30, 2020
// MJ_CONNECTION
MN_CONNLOST = 1,
MN_NOCONN = 2,
// MJ_SYSTEMRES
MN_THREAD = 1,
MN_MEMORY = 2,
MN_OBJECT = 3,
Copy link
Collaborator

Choose a reason for hiding this comment

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

MN_OBJECT is not used in the code (never returned).Something for the future? Do we need to add it then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's another PR that is going to fix problems around the unreported errors from the Mutex/CV creation functions. There are these errors reported. Passing it here might spare too much noise in that PR.

@maxsharabayko maxsharabayko modified the milestones: v1.5.0 - Sprint 18, v1.5.0 - Sprint 19 Jul 14, 2020
@maxsharabayko maxsharabayko modified the milestones: v1.5.0 - Sprint 19, v1.5.0 - Sprint 20 Jul 27, 2020
Comment on lines 1885 to 1888
This error should be reported by the sending function in case when
with `SRTO_TLPKTDROP` set to true there were packets had to be removed
from the sender buffer and forgotten because the network is unable
to live up to the sending rate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This error should be reported by the sending function in case when
with `SRTO_TLPKTDROP` set to true there were packets had to be removed
from the sender buffer and forgotten because the network is unable
to live up to the sending rate.
This error should be reported by the sending function when, with
`SRTO_TLPKTDROP` set to true, packets to be removed from the
sender buffer are forgotten because the network is unable
to support to the sending rate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll try to describe the situation. When this error is encountered:

  • SRTO_TLPKTDROP flag was set
  • the TLPKTDROP on the sender site was triggered (there were found packets in the sender buffer, which's predicted delivery time is already in the past, and they are still not acknowledged - in that case the sender simply drops these packets from the sender buffer and they are no longer eligible for retransmission)

And also the SRT_ENABLE_ECN was turned on at compile time.

In this case the data sending function will return this error, however the data that it has scheduled for sending have been accepted.

Maybe this way:

This error should be reported by the sending function when, with SRTO_TSBPDMODE and SRTO_TLPKTDROP set to true, some packets were dropped at the sender side (see description for SRTO_TLPKTDROP` for details). This doesn't concern the data that were passed for sending by this call (these data are placed at the back side of the sender buffer, while the dropped packets are at the front side of the sender buffer). In other words, the operation done by the sending function is actually successful, just the application might want to slowdown the sending rate to avoid the congestion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your last suggestion is much better. Some minor tweaks:

This error should be reported by the sending function when, with SRTO_TSBPDMODE and SRTO_TLPKTDROP set to true, some packets were dropped at the sender side (see the description of SRTO_TLPKTDROP for details). This doesn't concern the data that were passed for sending by the sending function (these data are placed at the back of the sender buffer, while the dropped packets are at the front). In other words, the operation done by the sending function is successful, but the application might want to slow down the sending rate to avoid congestion.

srtcore/api.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@stevomatthews stevomatthews left a comment

Choose a reason for hiding this comment

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

Minor edits.

ethouris and others added 3 commits July 30, 2020 13:33
Doc review followup, phase 1.

Co-authored-by: stevomatthews <smatthews@haivision.com>
Doc review followup, phase 1 (missed parts)

Co-authored-by: stevomatthews <smatthews@haivision.com>
Copy link
Collaborator

@stevomatthews stevomatthews left a comment

Choose a reason for hiding this comment

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

Approved with minor edits.

@maxsharabayko maxsharabayko merged commit 3c334a0 into Haivision:master Jul 30, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 20, v1.4.2 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core [docs] Area: Improvements or additions to documentation Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Clean up rules of error reports in srt_accept
4 participants