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

Use communicate() instead of wait() after killing the mock server #4095

Merged
merged 5 commits into from Oct 31, 2019

Conversation

Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Oct 22, 2019

Fixes #4014

When using pipes, and we are using a pipe for stdout in MockServer, you are meant to call process.communicate() instead of process.wait().

This was causing unexpected warnings in test_crawler.py tests, when the garbage collection happened to warn about unclosed resources while one of the affected tests was capturing warnings.

I’ve run the tests a few rounds with this fix, and it seems to have worked.

@Gallaecio Gallaecio changed the title Log the text of extra warnings that are randomly captured [WIP] Log the text of extra warnings that are randomly captured Oct 24, 2019
@Gallaecio Gallaecio closed this Oct 24, 2019
@Gallaecio Gallaecio reopened this Oct 24, 2019
@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

Merging #4095 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4095   +/-   ##
=======================================
  Coverage   85.68%   85.68%           
=======================================
  Files         165      165           
  Lines        9734     9734           
  Branches     1463     1463           
=======================================
  Hits         8341     8341           
  Misses       1136     1136           
  Partials      257      257

@Gallaecio Gallaecio changed the title [WIP] Log the text of extra warnings that are randomly captured [WIP] Use communicate() instead of wait() after killing the mock server Oct 24, 2019
@Gallaecio Gallaecio added bug and removed enhancement labels Oct 24, 2019
@Gallaecio Gallaecio closed this Oct 24, 2019
@Gallaecio Gallaecio reopened this Oct 24, 2019
@Gallaecio Gallaecio changed the title [WIP] Use communicate() instead of wait() after killing the mock server Use communicate() instead of wait() after killing the mock server Oct 24, 2019
Copy link
Member

@elacuesta elacuesta left a comment

Nice 💪

tests/mockserver.py Outdated Show resolved Hide resolved
wRAR
wRAR approved these changes Oct 31, 2019
@Gallaecio Gallaecio merged commit f02c3d1 into scrapy:master Oct 31, 2019
2 checks passed
@s-sanjay
Copy link
Contributor

s-sanjay commented Oct 31, 2019

@Gallaecio how did you root cause the issue to this test ?

@Gallaecio
Copy link
Member Author

Gallaecio commented Nov 1, 2019

@Gallaecio how did you root cause the issue to this test ?

It took more time that I would like to admit to figure things out.

In the end, search engine results for similar issues started to suggest, based on the naming (positive integers) of the unclosed file descriptors (which we first knew thanks to your #4048, by the way!), that the files were created by subprocess pipes.

Once I reached that conclusion, locating the affected code was not hard. It was unlikely that we were using subprocess in Scrapy itself, so it had to be in the tests. And the MockServer was using subprocess with a pipe, and was used strongly in the test file executed right after the file with random failing tests.

Then it was a matter of finding out what was the right way to get pipe files closed, see how we were actually doing it, and hence this patch. After 4-5 runs of the patched build being unable to reproduce the issue, I decided it was very likely that this patch had fixed the issue (let’s hope it has).

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

Successfully merging this pull request may close these issues.

Random(?) test_crawler.py failure on Travis
4 participants