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

Thread safety using WSAOVERLAPPED #2838

Closed
maxsharabayko opened this issue Jan 2, 2024 · 4 comments
Closed

Thread safety using WSAOVERLAPPED #2838

maxsharabayko opened this issue Jan 2, 2024 · 4 comments
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@maxsharabayko
Copy link
Collaborator

PR #2834 has a problem, The srt::CChannel::sendto will be call in CSndQueue(thread1) and CRcvQueue(thread2), and that not thread safety.

Them used same WSAOVERLAPPED, so WSASendTo maybe not completed when WSAWaitForMultipleEvents return.

From https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsasendto

The lpOverlapped parameter must be valid for the duration of the overlapped operation. If multiple I/O operations are simultaneously outstanding, each must reference a separate WSAOVERLAPPED structure.

Originally posted by @mGaosi in #2834 (comment)

@maxsharabayko maxsharabayko added this to the v1.5.4 milestone Jan 2, 2024
@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Jan 2, 2024
@mGaosi
Copy link
Contributor

mGaosi commented Jan 2, 2024

We found this issue during our app testing. When sending 400mbps of data in our program, some exceptions quickly occurred and the stack was damaged. The probability of this problem occurring is related to your actual traffic and the performance of the network subsystem.

I am currently using a temporary solution, using TLS WSAEvent and stack WSAOVERLAPPED.

    WSAOVERLAPPED overlapped;
    ::SecureZeroMemory(&overlapped, sizeof(overlapped));
    class WSAEventRef
    {
    public:
        WSAEventRef()
            : e(::WSACreateEvent())
        {
        }
        ~WSAEventRef()
        {
            ::WSACloseEvent(e);
            e = NULL;
        }
        void reset()
        {
            ::WSAResetEvent(e);
        }
        WSAEVENT handle()
        {
            return e;
        }

    private:
        WSAEVENT e;
    };
    thread_local WSAEventRef lEvent;
    overlapped.hEvent = lEvent.handle();
    int res = ::WSASendTo(m_iSocket, (LPWSABUF)packet.m_PacketVector, 2, &size, 0, addr.get(), addrsize, &overlapped, nullptr);

@maxsharabayko
Copy link
Collaborator Author

The Windows build of SRT enables C++11 by default. If disabled, the pthread port is being used. But it is waiting to be deprecated.

@mGaosi
I believe your solution works well and is worth to make a PR out of it. I would just do something like this:

#if HAVE_CXX11
thread_local WSAEventRef lEvent;
#else
WSAEventRef lEvent;
#endif

P.S. Look like HAVE_CXX11 and ENABLE_CXX11 are not working together. 🤔

@ethouris
Copy link
Collaborator

If we state that Windows can only be built with C++11, then it should suffice to declare thread_local WSAEventRef lEvent under a condition for WIN32 only.

mGaosi added a commit to mGaosi/srt that referenced this issue Jan 22, 2024
…#2838).

The lpOverlapped parameter must be valid for the duration of the overlapped operation. If multiple I/O operations are simultaneously outstanding, each must reference a separate WSAOVERLAPPED structure.

This reverts commit b1c0be2.

resolves Haivision#2632 Haivision#2834 Haivision#2838
mGaosi added a commit to mGaosi/srt that referenced this issue Jan 22, 2024
…#2838).

The lpOverlapped parameter must be valid for the duration of the overlapped operation. If multiple I/O operations are simultaneously outstanding, each must reference a separate WSAOVERLAPPED structure.

This reverts commit b1c0be2.

resolves Haivision#973 Haivision#2632 Haivision#2834 Haivision#2838
mGaosi added a commit to mGaosi/srt that referenced this issue Jan 26, 2024
…#2838).

The lpOverlapped parameter must be valid for the duration of the overlapped operation. If multiple I/O operations are simultaneously outstanding, each must reference a separate WSAOVERLAPPED structure.

This reverts commit b1c0be2.

resolves Haivision#973 Haivision#2632 Haivision#2834 Haivision#2838
maxsharabayko added a commit to maxsharabayko/srt that referenced this issue Apr 16, 2024
The lpOverlapped parameter must be valid for the duration of the overlapped operation.
If multiple I/O operations are simultaneously outstanding, each must reference a separate WSAOVERLAPPED structure.
Resolves Haivision#973, Haivision#2632, Haivision#2834, Haivision#2838.

Co-authored-by: Jiangjie Gao <gaojiangjie@live.com>
maxsharabayko added a commit to maxsharabayko/srt that referenced this issue Apr 16, 2024
The lpOverlapped parameter must be valid for the duration of the overlapped operation.
If multiple I/O operations are simultaneously outstanding, each must reference a separate WSAOVERLAPPED structure.
Resolves Haivision#973, Haivision#2632, Haivision#2834, Haivision#2838.

Co-authored-by: Jiangjie Gao <gaojiangjie@live.com>
maxsharabayko added a commit to maxsharabayko/srt that referenced this issue Apr 17, 2024
The lpOverlapped parameter must be valid for the duration of the overlapped operation.
If multiple I/O operations are simultaneously outstanding, each must reference a separate WSAOVERLAPPED structure.
Resolves Haivision#973, Haivision#2632, Haivision#2834, Haivision#2838.

Co-authored-by: Jiangjie Gao <gaojiangjie@live.com>
maxsharabayko added a commit that referenced this issue Apr 17, 2024
The lpOverlapped parameter must be valid for the duration of the overlapped operation.
If multiple I/O operations are simultaneously outstanding, each must reference a separate WSAOVERLAPPED structure.
Resolves #973, #2632, #2834, #2838.

Co-authored-by: Jiangjie Gao <gaojiangjie@live.com>
@maxsharabayko
Copy link
Collaborator Author

Resolved by #2929.

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

No branches or pull requests

3 participants