[webkit-reviews] review granted: [Bug 172774] test-webkitpy: stop forking unsafely from within a spawned process : [Attachment 311657] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Dean Johnson
<dean_johnson at apple.com>'s request for review:
Bug 172774: test-webkitpy: stop forking unsafely from within a spawned process
https://bugs.webkit.org/show_bug.cgi?id=172774

Attachment 311657: Patch

https://bugs.webkit.org/attachment.cgi?id=311657&action=review




--- Comment #6 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 311657
  --> https://bugs.webkit.org/attachment.cgi?id=311657
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311657&action=review

Nice temporary hack! This of course relies on not testing the force_build
function, which is not great, and on not testing any other code touching
objects that are unsafe to use while forking.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:135
> +	   # Initialize here since Mechanize will spawn a new process for
Browser, which is
> +	   # incompatible with using Python's multiprocessing safely.

I think that this comment may be a bit too vague. How about something like
"Creating Browser lazily avoids doing that as part of test-webkitpy, which is
unsafe due to other tests using Python's multiprocessing"?

My comment says something different than yours, and I'm not entirely sure which
is right. I don't see how the forking in Mechanize would be a problem, I think
that it's regular run-webkit-tests forking that becomes a problem.