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

Fixes race condition getting IPv4 TCP table on Windows. #880

Merged
merged 2 commits into from
Sep 11, 2016
Merged

Fixes race condition getting IPv4 TCP table on Windows. #880

merged 2 commits into from
Sep 11, 2016

Conversation

AndreLouisCaron
Copy link
Contributor

We've been getting rather frequent cases where the psutil.Process().connections() returns an empty list on Windows when we know for a fact that there are open TCP connections on the machine.

We traced down the problem to the fact that there is a race condition in the GetExtendedTCPTable() API: since we call it twice (once to get the size, once to read the data), it happends that new connections open between the two calls to the GetExtendedTCPTable() function and the table size is too small on the 2nd call.

The solution to this is to call the function in a loop and repeat until we manage to successfully read the data.

I couldn't run the tests on my machine, so I'm sending this as a preliminary patch. I think the other calls (e.g. for IPv6, and probably UDP) also need this too. Maybe some refactoring could help move this logic in common somewhere.

@giampaolo
Copy link
Owner

This looks reasonable.
Given that this may loop different times I think it makes sense to release the GIL. You can do so by using Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around GetExtendedTCPTable.
Also you should do the same for GetExtendedUDPTable.
Also please update HISTORY and CREDITS file.

@AndreLouisCaron
Copy link
Contributor Author

OK, thanks, will update shortly :-)

@AndreLouisCaron
Copy link
Contributor Author

OK. I updated the code to affect TCP IPv6, UDP IPv4 and UDP Ipv6 too. I also updated HISTORY.rst and CREDITS.

Tests are not passing now, still needs a bit of work.

@AndreLouisCaron
Copy link
Contributor Author

Added the code to release the GIL, as requested :-)

@AndreLouisCaron
Copy link
Contributor Author

@giampaolo I think I addressed everything you asked. Let me know if this PR needs more work :-)

@giampaolo
Copy link
Owner

Sorry I'm busy and still didn't have time to look at this but I promise I will (likely during the week end)

@giampaolo giampaolo merged commit 601f959 into giampaolo:master Sep 11, 2016
@giampaolo
Copy link
Owner

Merged. Thanks.

@AndreLouisCaron
Copy link
Contributor Author

Thanks for merging this!

@giampaolo
Copy link
Owner

Thanks for the PR. ;)
I will make a release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants