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

[core] Fixed a bug that responded a repeated conclusion HS with rejection #1650

Merged
merged 8 commits into from
Dec 10, 2020

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Nov 9, 2020

Fixes Case 1 in #1648

@ethouris ethouris changed the title [core] Fixed a bug that responded a repeated conclusion HS with rejec… [core] Fixed a bug that responded a repeated conclusion HS with rejection Nov 9, 2020
@ethouris ethouris added [core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior labels Nov 9, 2020
@codecov

This comment has been minimized.

@ethouris ethouris marked this pull request as ready for review November 26, 2020 16:04
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.

Are both case 1 and case 2 of #1648 resolved by thes changes, or only case 1?

srtcore/api.h Show resolved Hide resolved
@ethouris
Copy link
Collaborator Author

Tested with case 1 only. I think case 2 is not covered because if a caller is "not convinced to be connected" and therefore still sends HS packets, it won't get any answer because the accepted socket won't send the answer simply because no one will dispatch the HS packet to it, as per having id=0. This requires another fix.

@maxsharabayko
Copy link
Collaborator

Updated description so that #1648 is not automatically closed after merging this PR.

@mbakholdina mbakholdina added this to the v1.4.3 milestone Nov 27, 2020
@maxsharabayko
Copy link
Collaborator

maxsharabayko commented Dec 1, 2020

A repeated HS CONCLUSION response has:

  • Unset Destination socket ID (the value set is 0 instead of the known value of the caller socket)
  • Weird Flow Window value 25600 instead of 8192.
  • A timestamp copied from HS request instead of the listener's socket TS.

@maxsharabayko
Copy link
Collaborator

@ethouris Could you please set e.g. 2 seconds of RTT on some link and collect a network capture of the handshake to confirm the issue is resolved?
This will make a listener repeat its conclusion handshakes, and we will see if the fields listed above are correctly filled.

@maxsharabayko
Copy link
Collaborator

Had to perform the test myself. Confirmed that repeated Conclusion Response handshakes have valid content, including correct KMX message.

@maxsharabayko maxsharabayko merged commit 0a61cb9 into Haivision:master Dec 10, 2020
maxsharabayko added a commit to maxsharabayko/srt that referenced this pull request Apr 4, 2022
Update listener write-ready only after the new connection.
Was changed in Haivision#1650, but must not be done at all (see Haivision#1831).
maxsharabayko added a commit to maxsharabayko/srt that referenced this pull request Apr 4, 2022
Update listener write-ready only after the new connection.
Was changed in Haivision#1650, but must not be done at all (see Haivision#1831).
maxsharabayko added a commit to maxsharabayko/srt that referenced this pull request Apr 12, 2022
Update listener write-ready only after the new connection.
Was changed in Haivision#1650, but must not be done at all (see Haivision#1831).
maxsharabayko added a commit that referenced this pull request Apr 21, 2022
Update listener write-ready only after the new connection.
Was changed in #1650, but must not be done at all (see #1831).
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: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants