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] Fix for srt_connect() in the blocking mode #833

Merged
merged 3 commits into from
Aug 27, 2019

Conversation

maxsharabayko
Copy link
Collaborator

Under certain conditions m_bConnecting variable on a socket might not be reset to false after a connection attempt has failed.
In that case any further call to srt_connect will return SRT_ECONNSOCK:
"Operation not supported: Cannot do this operation on a CONNECTED socket"

In general case the receiver's worker thread sets m_bConnecting to false on timeout. That thread checks for a timeout roughly every 10 ms.

There is another check for TTL in the srt_connect function itself:

if (CTimer::getTime() > ttl)
{
	// timeout
	e = CUDTException(MJ_SETUP, MN_TIMEOUT, 0);
	break;
}

That check happens every second (or on a spurious wake up on a conditional variable m_PassCond in CRcvQueue::recvfrom()).

In case it was srt_connect thread, that handles the connection timeout, it does not reset the m_bConnecting variable to false. In that case any further call to srt_connect will fail to connect.

A regression unit test is added, although it is a matter of probability whether it catches the issue.

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Aug 23, 2019
@maxsharabayko maxsharabayko added this to the v1.4.0 milestone Aug 23, 2019
@maxsharabayko maxsharabayko requested a review from ethouris August 23, 2019 11:28
@maxsharabayko maxsharabayko force-pushed the hotfix/connection_loop branch from b02c8d7 to 6c4c4de Compare August 23, 2019 11:36
sockaddr_in sa;
memset(&sa, 0, sizeof sa);
sa.sin_family = AF_INET;
sa.sin_port = htons(5555);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest making the 5555 port surely not bound with an SRT listener and surely not used by any other service. Otherwise the test may be thwarted by a service occupying port 5555.

To achieve that, you may start with 5000 and probably go up with the number while trying to bind a UDP socket to this port. Once successful, you can use this port in the test, then release it. For UDP it doesn't matter if anyone is reading from the port into which packets are otherwise coming.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ethouris Added UDP port check before connection timeout tests.

Copy link
Collaborator

@ethouris ethouris left a comment

Choose a reason for hiding this comment

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

All ok, consider preserving the port before test.

@maxsharabayko maxsharabayko force-pushed the hotfix/connection_loop branch from 6120b37 to a4277cd Compare August 26, 2019 14:37
@maxsharabayko maxsharabayko force-pushed the hotfix/connection_loop branch from a4277cd to 596062e Compare August 26, 2019 14:46
@rndi rndi merged commit 92dc53c into Haivision:master Aug 27, 2019
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