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

Reset client gateway reciever buffer on socket reset. #2316

Merged
merged 1 commit into from Oct 20, 2016

Conversation

jason-bragg
Copy link
Contributor

@jason-bragg jason-bragg commented Oct 18, 2016

Networking receiver buffer must be reset on socket error or reset. Since this can happen on a send rather than a receive, it is now possible for the socket to be reset by the send without the receive buffer being cleared, which can lead to buffer corruption.
This change resets the receive buffer whenever the socket changes.

@gabikliot
Copy link
Contributor

There is a good change that the fix is correct, but there is also a nonzero chance it is not.
The new pattern seems to be very fragile to me: there is now a protocol (albeit louse, but still) between the sender and receiver that if sender resets the socket, the receiver needs to do some kind of cleanup.

Once you have such a protocol, a good protocol design will make it explicit (sender tells the receiver in some way), instead of being implicit via socket mutating under the covers. I call this non robust protocol.
I remember way way back in my youth (:-)) spending multiple days chasing a similar bug with C sockets in Linux, when the socket was a file descriptor and it was reused and happened sometimes to be the same integer number but different logical socket. It was a nightmare to find.

So I may be too harsh, or maybe saving you countless debugging hours, but my suggestion is to switch to an explicit protocol (make sender know about the receiver, or subscribe receiver as a listeners to sender sending socket reset events, what ever protocol you design ...).

@gabikliot
Copy link
Contributor

http://research.microsoft.com/en-us/um/people/blampson/33-Hints/WebPage.html

I reread it periodically, once every couple of years, to remind myself. We tend to forget.

@jason-bragg
Copy link
Contributor Author

jason-bragg commented Oct 19, 2016

@gabikliot, I appreciate the thought you put into this, but I disagree with your conclusion. It's a question of framing.

I don't see this as a protocol between the send and receive logic. I see it as a relationship between the receive logic and the gateway connection. The receive logic has always (or at least as long as I've maintained this code) accounted for the fact that the gateway connection could be modified by another thread. This is why a local socket was previously being cached prior to the send. This is not a new expectation, nor does the send side know about these expectations, as it has it's own expectations on the gateway connection.

This change is an update to the receive logic to account for existing gateway connection expectations. I see no reason for the send logic to have any knowledge or communicate with the receive logic.

@gabikliot gabikliot merged commit 76958c2 into dotnet:master Oct 20, 2016
@gabikliot
Copy link
Contributor

👍

sergeybykov pushed a commit to sergeybykov/orleans that referenced this pull request Oct 27, 2016
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants