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

Fixed incorrectly interpreted error from WSARecvFrom on Windows #124

Merged
merged 5 commits into from
Oct 10, 2017

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Oct 9, 2017

There was a change added to error interpretation from recvmsg system call, but this was unfortunately added the same way for the Windows system call of WSARecvFrom. It uses completely different set of error codes (this should be further studied, but the Windows implementation also needs strong refactoring, so this is temporarily put aside). Just to clear the problem, the "old way" is used on windows, that is, every error reported by WSARecvFrom is treated as a "potential AGAIN" situation.

@@ -441,6 +441,45 @@ EReadStatus CChannel::recvfrom(sockaddr* addr, CPacket& packet) const

int res = ::recvmsg(m_iSocket, &mh, 0);
int msg_flags = mh.msg_flags;


// Note that there are exactly four groups of possible errors
Copy link
Collaborator

Choose a reason for hiding this comment

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

idents for the entire block below

int res = ::WSARecvFrom(m_iSocket, (LPWSABUF)packet.m_PacketVector, 2, &size, &flag, addr, &addrsize, NULL, NULL);
res = (0 == res) ? size : -1;
int sockerror = ::WSARecvFrom(m_iSocket, (LPWSABUF)packet.m_PacketVector, 2, &size, &flag, addr, &addrsize, NULL, NULL);
int res = (0 == sockerror) ? size : -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

idents in the entire block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reindented whole function

// WSAETIMEDOUT, which isn't mentioned in the documentation of WSARecvFrom at all.
int err = NET_ERROR;
LOGC(mglog.Debug) << CONID() << "(sys)WSARecvFrom: " << SysStrError(err) << " [" << err << "]";
status = RST_AGAIN;
Copy link
Collaborator

@rndi rndi Oct 9, 2017

Choose a reason for hiding this comment

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

What will happen if network goes away, ex. cable gets unplugged?
Why not put a switch here that checks for relevant error codes reported by WSAGetLastError() in case an error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is completely explained in the initial description. In case when the network goes down, the reader will retry reading indefinitely - which is how the older version were behaving (with the difference that stransmit on Windows also doesn't implement the data passing timeout feature correctly due to nonexistent alarm()). Of course, this should be fixed, but the fix that is needed for Windows here is much bigger and deserves a more effort consuming activity (everything is explained in comments around it).

@rndi rndi merged commit 13df2de into Haivision:dev Oct 10, 2017
@rndi
Copy link
Collaborator

rndi commented Oct 10, 2017

Thank you @ethouris !

@ethouris ethouris deleted the dev-fix-windows-recvagain branch January 17, 2018 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants