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 an infinite read loop with SRV record resolution on windows #25672

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

apolcyn
Copy link
Contributor

@apolcyn apolcyn commented Mar 10, 2021

Tentative fix for #25654

One effect of the simplification in #25105 was that it changed the c-ares resolver to invoke RegisterOnReadableLocked on all active fd's also now when SRV records are resolved. Notably, it does this from a callback that is invoked from the c-ares library during a call to ares_process, and it also clears the readable_registered_ flag before doing this. After this ares_process call completes, we check if there are still more bytes to read on the socket and then continue looping on ares_process if so.

On Windows, RegisterOnReadableLocked pre-allocates space for read_buf_ to read in bytes asynchronously via RecvFrom. Data is actually filled into read_buf_ only when the RecvFrom call completes and the OnIocpReadableLocked callback is invoked. So because we are calling RegisterForOnReadableLocked from a callback invoked from ares_process, when ares_process returns, read_buf_ has a non-zero length but doesn't actually have any readable data. Thus the loop where we continue calling ares_process so long as IsStillReadableLocked returns true will never finish.

read_buf_has_data_ is set true when we read in data from RecvFrom, and is then cleared after c-ares reads all received bytes out of it (this is the proper flag to check if there are still bytes remaining to be read).

@apolcyn apolcyn requested a review from markdroth as a code owner March 10, 2021 18:28
@apolcyn apolcyn added release notes: yes Indicates if PR needs to be in release notes lang/core labels Mar 10, 2021
@apolcyn
Copy link
Contributor Author

apolcyn commented Mar 10, 2021

interop cloud to cloud: #25679

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/core release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants