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

Added explicit closing the sockets. To prevent messing with UDP sockets keeping ports busy. #2807

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Oct 2, 2023

The reason why this is necessary is that it has been observed in some test CI environments that sometimes sockets fail to bind a port, for which the reason could be that the sockets may be deleted with too large delay and cause the next test to fail due to inability to bind the port.

@maxsharabayko maxsharabayko added this to the v1.6.0 milestone Oct 8, 2023
@maxsharabayko maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [tests] Area: Unit tests labels Oct 8, 2023
@maxsharabayko
Copy link
Collaborator

This PR does not look clean. Instead of closing a socket in the testing function itself, a kind of a "garbage socket collector" is being created, that accumulates created sockets and closes them in the end. This code distracts and complicates the testing code.

For example, here the client_socket could just be closed in the end. The test uses ASSERT instead of EXPECT, so that once it fires, the function does not reach the end and the socket won't be closed. However, almost every test uses the TestInit class after PR #2681, which calls srt_cleanup() in its destructor. The srt-cleanup() function is expected to close all open sockets.

If a socket should be closed anyway, It may be cleaner to use some smart socket object, that would close the socket in its destructor, and can be converted to SRT_SOCKET when passed to SRT API functions directly.

class unique_srt_socket
{
public:
    unique_srt_socket(SRTSOCKET s);
   ~unique_srt_socket();
   // ...
};

@maxsharabayko maxsharabayko merged commit c506764 into Haivision:master Jan 16, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[tests] Area: Unit tests Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants