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

Fix TCP Connection data receive race condition #1578

Merged
merged 1 commit into from
Feb 13, 2017

Conversation

dipinhora
Copy link
Contributor

Prior to this commit, when we _accepted a new connection
or completed a connection via _event_notify there was a race
condition around reading data that the other party sent to us.

This existed because we didn't check to see if any data was
waiting on the socket for us already and instead only subscribed
to ASIO events for new read notifications. If the other party sent
us data after we signed up for ASIO events, we would successfully
receive notification and read the data. However, if the other
party sent us data before we signed up for ASIO events, we would
not read the data already on the socket and would wait for an ASIO
event telling us that we have data to read.

This commit fixes this race condition by ensuring that we try to
read from the socket whenever we _accept or complete a connection
via _event_notify prior to signing up for ASIO events. This
ensures we will not accidentally ignore any data that may already
be on the socket by the time we're ready to handle the connection.

Prior to this commit, when we `_accept`ed a new connection
or completed a connection via `_event_notify` there was a race
condition around reading data that the other party sent to us.

This existed because we didn't check to see if any data was
waiting on the socket for us already and instead only subscribed
to ASIO events for new read notifications. If the other party sent
us data after we signed up for ASIO events, we would successfully
receive notification and read the data. However, if the other
party sent us data before we signed up for ASIO events, we would
not read the data already on the socket and would wait for an ASIO
event telling us that we have data to read.

This commit fixes this race condition by ensuring that we try to
read from the socket whenever we `_accept` or complete a connection
via `_event_notify` prior to signing up for ASIO events. This
ensures we will not accidentally ignore any data that may already
be on the socket by the time we're ready to handle the connection.
@dipinhora
Copy link
Contributor Author

I forgot to mention that this change makes the TCPMute, TCPUnmute and TCPThrottle test more reliable. They would sometimes previously fail because of the race condition around receiving data resolved by this commit.

@jemc
Copy link
Member

jemc commented Feb 13, 2017

The one failure in the CI matrix was a Travis blip, and unrelated to these changes. I'm merging.

@jemc jemc added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Feb 13, 2017
@jemc jemc merged commit 9a60bd1 into ponylang:master Feb 13, 2017
ponylang-main added a commit that referenced this pull request Feb 13, 2017
dipinhora added a commit to WallarooLabs/wally that referenced this pull request Sep 28, 2017
Backport ponylang/ponyc#1578 and ponylang/ponyc#1626 to net actors
because the help resolve edge cases around the networking code with
direct impacts to the stability of the network tests.
nisanharamati pushed a commit to WallarooLabs/wally that referenced this pull request Sep 29, 2017
Backport ponylang/ponyc#1578 and ponylang/ponyc#1626 to net actors
because the help resolve edge cases around the networking code with
direct impacts to the stability of the network tests.
dipinhora added a commit to WallarooLabs/wally that referenced this pull request Oct 5, 2017
Backport ponylang/ponyc#1578 and ponylang/ponyc#1626 to net actors
because the help resolve edge cases around the networking code with
direct impacts to the stability of the network tests.
JONBRWN pushed a commit to WallarooLabs/wally that referenced this pull request Oct 12, 2017
Backport ponylang/ponyc#1578 and ponylang/ponyc#1626 to net actors
because the help resolve edge cases around the networking code with
direct impacts to the stability of the network tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants