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 some problems in group code found during review #1505

Merged
merged 8 commits into from
Sep 2, 2020

Conversation

ethouris
Copy link
Collaborator

Things found during the review:

  1. Synchronization group data with the socket data at the caller side:
  • wrong reaction on isn == 0 (0 is a perfectly legal sequence value), which likely was remaining after the fact that a freshly created group has recorded scheduling sequence undefined
  • the right "undefined sequence" value is -1, but this no longer makes sense because the group is now always initialized the schedule sequence number due to another bugfix
  • the intention was to synchronize the latency value for the group from the value taken from the first socket in case when the group didn't get this information yet. The condition to do it was then changed to simply checking if the group is empty. Also the synchronization done at the caller side has been changed to not perform the sequence number derivation, as it doesn't make sense here - now the ISN is created in the group and it's the socket deriving this value from group, not the other way around
  1. The use of getMasterData has proven to be now informative only - therefore the whole call is put under ENABLE_HEAVY_LOGGING condition for clarity.

  2. Comparison of the expected peer id in case when the group is not empty should result in rejection. This is a probable situation if the user has mistakenly made two connections within a group, but these endpoints do not designate the same application at the peer side. In this case the returned peer group ID will be different for the second member socket, and this means that the address of the second connection is wrong, stating that the first was right.

@ethouris ethouris added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core Impact: High labels Aug 28, 2020
@ethouris ethouris added this to the v1.5.0 - Sprint 22 milestone Aug 28, 2020
srtcore/api.cpp Outdated Show resolved Hide resolved
srtcore/core.cpp Outdated Show resolved Hide resolved
srtcore/core.cpp Outdated Show resolved Hide resolved
Comment on lines +1865 to +1870
// NOTE: this code remains as is for historical reasons.
// The initial implementation stated that the peer id be
// extracted so that it can be reported and possibly the
// start time somehow encoded and written into the group
// extension, but it was later seen not necessary. Therefore
// this code remains, but now it's informational only.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it valid now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. This explains why this call is here.

@maxsharabayko maxsharabayko merged commit 633da74 into Haivision:master Sep 2, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 22, 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 Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants