Bug 77781 – webkitpy: change exit() calls to sys.exit(), fix a leak in outputcapture

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

webkitpy: change exit() calls to sys.exit(), fix a leak in outputcapture

[Dirk Pranke]

Created attachment 125426 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 125426 [details]
Patch

http://docs.python.org/library/sys.html#sys.exit

So I guess the downside of calling exit() is that it only works from the main thread?

[Eric Seidel (no email)]

Comment on attachment 125426 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/outputcapture.py:84
> +        try:
> +            if expected_exception:
> +                return_value = testcase.assertRaises(expected_exception, function, *args, **kwargs)
> +            else:
> +                return_value = function(*args, **kwargs)
> +        finally:
> +            (stdout_string, stderr_string, logs_string) = self.restore_output()
> +

So when does this leak?  And how does this leak manifest?  I assume we hit this if an exception is thrown during a unittest?  The unittest framework will catch the exception for us and continue, but our output will never get restored?

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 125426 [details])
> http://docs.python.org/library/sys.html#sys.exit
> 
> So I guess the downside of calling exit() is that it only works from the main thread?

sys.exit(), yes. Fortunately, as far as I know, we don't use multiple threads anywhere in webkitpy :).

Note that there is a note in the python docs commenting that exit() shouldn't be used outside of the interpreter:

http://docs.python.org/library/constants.html

(In reply to comment #3)
> (From update of attachment 125426 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125426&action=review
> 
> > Tools/Scripts/webkitpy/common/system/outputcapture.py:84
> > +        try:
> > +            if expected_exception:
> > +                return_value = testcase.assertRaises(expected_exception, function, *args, **kwargs)
> > +            else:
> > +                return_value = function(*args, **kwargs)
> > +        finally:
> > +            (stdout_string, stderr_string, logs_string) = self.restore_output()
> > +
> 
> So when does this leak?

This leaked if expected_exception was not set or if expected_exception was the wrong class.  

> And how does this leak manifest?  I assume we hit this if an exception is thrown during a unittest?  The unittest framework will catch the exception for us and continue, but our output will never get restored?

Yup, you got it. I discovered it when I had a breakpoint set in the python debugger that would fire after all of tests were done being run; pdb would error out since stdin and stderr were messed up.

I have forgotten if the tests in question were actually written incorrectly and should be fixed; I can go back and check that as part of this if you like. I can also add a test to outputcapture_unittest for this if you like, but I'm not always a fan of writing tests for tests, so I was waffling and/or lazy :).

[Eric Seidel (no email)]

Comment on attachment 125426 [details]
Patch

seems oK.

[Dirk Pranke]

Committed r106721: <http://trac.webkit.org/changeset/106721>