Bug 77781 - webkitpy: change exit() calls to sys.exit(), fix a leak in outputcapture
Summary: webkitpy: change exit() calls to sys.exit(), fix a leak in outputcapture
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-03 14:41 PST by Dirk Pranke
Modified: 2012-02-03 17:41 PST (History)
4 users (show)

See Also:


Attachments
Patch (10.83 KB, patch)
2012-02-03 14:56 PST, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-02-03 14:41:10 PST
webkitpy: change exit() calls to sys.exit(), fix a leak in outputcapture
Comment 1 Dirk Pranke 2012-02-03 14:56:07 PST
Created attachment 125426 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-03 16:24:20 PST
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?
Comment 3 Eric Seidel (no email) 2012-02-03 16:28:18 PST
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?
Comment 4 Dirk Pranke 2012-02-03 16:36:46 PST
(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 :).
Comment 5 Eric Seidel (no email) 2012-02-03 16:57:07 PST
Comment on attachment 125426 [details]
Patch

seems oK.
Comment 6 Dirk Pranke 2012-02-03 17:41:26 PST
Committed r106721: <http://trac.webkit.org/changeset/106721>