Bug 172774 – test-webkitpy: stop forking unsafely from within a spawned process

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dean Johnson]

webkitpy when running unit tests forks unsafely from within a spawned process while running the tests.

[Dean Johnson]

rdar://problem/32358666

[Radar WebKit Bug Importer]

<rdar://problem/32494848>

[Dean Johnson]

Created attachment 311630 [details]
Patch

[Dean Johnson]

Created attachment 311657 [details]
Patch

[Dean Johnson]

Last patch changes instantiation of self._browser by instantiating it as 'None' in the __init__ function, then actually assigning it a mechanize.Browser() object if we attempt to use self._browser (in Builder.force_build) when it hasn't been initialized.

This is similar to how access is done for statusserver and bugzilla interactions.

[Alexey Proskuryakov]

Comment on attachment 311657 [details]
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.

[Dean Johnson]

(In reply to Alexey Proskuryakov from comment #6)
> Comment on attachment 311657 [details]
> 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.
Actually, bugzilla and statusserver tests use MockBrowser() objects for testing so I think we could still test this codepath if we wanted, and all we'd need to do is something like this:

def test_force_build(self):
    from webkitpy.<something> import MockBrowser
    builder = Builder('test_builder')
    mock_browser = MockBrowser(args)
    # Setup return functions for Mock
    builder._browser = mock_browser
    builder.force_build()
    # Validate things ran as expected
> 
> > 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.

I feel my comment is more descriptive as to _why_ the logic now lives where it does, but I agree it could be improved upon. Maybe something like this would be better:

"Python2.7's multiprocessing module does not safely allow of spawning processes from within already spawned multiprocessing.Process objects. Because testwebkit-py uses multiprocessing.Process based Runners to execute tests quickly, we can not safely instantiate a Mechanize browser while running webkitpy tests."

[Dean Johnson]

Created attachment 311890 [details]
Patch

[Dean Johnson]

Removed comments Alexey and I were discussing since they were incorrect.

[WebKit Commit Bot]

Comment on attachment 311890 [details]
Patch

Clearing flags on attachment: 311890

Committed r217855: <http://trac.webkit.org/changeset/217855>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.