[core] Fixed thread safety using WSAOVERLAPPED in WSASendTo(#2838). #2861
Conversation
maxsharabayko
added
Type: Bug
|
The |
|
I am currently unable to reproduce issue of PR#2834 in our case. Maybe:
|
|
The hang up is reproducible when transferring files from a Windows machine using |
|
I have another idea: DWORD size = (DWORD)(CPacket::HDR_SIZE + packet.getLength());
int addrsize = addr.size();
int res = 0;
while (true)
{
res = ::WSASendTo(m_iSocket, (LPWSABUF)packet.m_PacketVector, 2, &size, 0, addr.get(), addrsize, NULL, NULL);
if (res == 0)
{
res = size;
break;
}
else if (WSAGetLastError() == WSAEWOULDBLOCK)
{
res = ::WSAEventSelect(m_iSocket, m_event, FD_READ | FD_WRITE);
if (res != 0)
{
LOGC(kslog.Warn, log << "CChannel::sendto WSAEventSelect failed " << NET_ERROR);
break;
}
}
else
{
break;
}
} |
|
@mGaosi I am testing your current proposal (this PR). Surprisingly it does not hang up. Looks like the problem was indeed in the handle that needed to be assigned. I'm still running some more iterations to ensure, but about 6 runs went without a single hang-up already. |
| #if HAVE_CXX11 | ||
| thread_local WSAEventRef lEvent; | ||
| #else | ||
| WSAEventRef lEvent; |
There was a problem hiding this comment.
Why do we keep the wrong solution in case of non-C++11 build?
If we state that the Windows version can only be compiled with C++11, and this feature is exclusively for Windows, then we don't need any alternative here.
If we allow C++03 with POSIX threads version compiled on Windows, then this alternative should contain a special type wrapper that would use a boolean marker operated with pthread_once.
There was a problem hiding this comment.
It is not completely wrong, it just creates and deletes a local object on each call for non-C++11 Windows builds.
This would also be the case for the minGW build.
Using pthread_once would be of course better.
There was a problem hiding this comment.
Ok, the situation is this: we can just as well disable "old" builds on MICROSOFT (that is, Windows with MSVC), but there is still possible MinGW with C++03, the possibility to compile successfully with C++11 (and C++11 sync) exists for MinGW, but this doesn't always work.
Still, I don't get why you apply thread_local onto a local variable.
There was a problem hiding this comment.
thread_local implies static when static is omitted. So it is thread_local static.
…#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
srtcore/channel.cpp
Outdated
| #if ENABLE_CXX11 | ||
| thread_local WSAEventRef lEvent; | ||
| #else |
There was a problem hiding this comment.
How about this?
| #if ENABLE_CXX11 | |
| thread_local WSAEventRef lEvent; | |
| #else | |
| #ifdef ENABLE_STDCXX_SYNC | |
| thread_local WSAEventRef lEvent; | |
| #else |
There was a problem hiding this comment.
This won't work, too, because the command line is not using C++11.
ENABLE macros configure what you want to have compiled, not what is possible to be compiled in the current env.
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 #973 #2632 #2834 #2838