Bug 175390 – Allow nested timeouts in webkitpy

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

[Jonathan Bedard]

Right now, we use alarms for timeouts.  This is a good technique in Python, but, has issues when timeouts need to be nested or multiple threads are used.  We should allow for timeouts to be nested and warn when timeouts are being used with multiple threads.

[Radar WebKit Bug Importer]

<rdar://problem/33803003>

[Jonathan Bedard]

Created attachment 317715 [details]
Patch

[Build Bot]

Attachment 317715 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/common/timeout_context.py:69:  [Timeout.__init__.exception_handler] Raising NoneType while only classes, instances or string are allowed  [pylint/E0702] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Comment on attachment 317715 [details]
Patch

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

r=me

Wow, this makes the code much more readable in simulated_device.py and simulator_process.py!

> Tools/Scripts/webkitpy/common/timeout_context.py:28
> +import os
> +import math
> +import logging
> +import signal
> +import time
> +import threading

Nit: Alphabetize.

> Tools/Scripts/webkitpy/common/timeout_context.py:56
> +            _log.critical('Using both alarms and threading in the same process')
> +            _log.critical('This is unsupported')

Nit: Any particular reason this can't be one log message?

> Tools/Scripts/webkitpy/common/timeout_context.py:61
> +        # seconds=0 is invalid

Nit: Shouldn't this be:

# seconds == 0 is invalid

> Tools/Scripts/webkitpy/common/timeout_context.py:62
> +        if seconds == 0:

Nit: If you created a helper method called secondsAreInvalid(seconds), then you can get rid of the comment:

        if self.secondsAreInvalid(seconds):

> Tools/Scripts/webkitpy/common/timeout_context.py:63
> +            raise RuntimeError('Cannot have a timeout of 0')

Nit:

            raise RuntimeError('Cannot have a timeout of 0 seconds')

> Tools/Scripts/webkitpy/common/timeout_context.py:97
> +        # Another timeout is more urgent

Nit: Comments should be sentences ending in periods.  (Please check other comments below.)

> Tools/Scripts/webkitpy/common/timeout_context.py:113
> +        signal.alarm(0)  # Imiediatly disable the alarm so we aren't interupted

Typos:  Imiediatly, interupted.
Nit: Period at end of sentence.

> Tools/Scripts/webkitpy/common/timeout_context_unittest.py:36
> +    def test_current_timeout(self):
> +        self.assertEqual(None, Timeout.current())
> +        with Timeout(1) as tmp:
> +            self.assertEqual(tmp.data, Timeout.current())
> +        self.assertEqual(None, Timeout.current())

This could be racy if the with: block doesn't execute in less than 1 second, correct?

This doesn't block landing the patch; just trying to understand how it works.

> Tools/Scripts/webkitpy/port/simulator_process.py:28
>  from webkitpy.port.server_process import ServerProcess
> -
> +from webkitpy.common.timeout_context import Timeout

Nit: Do we alphabetize by dotted path, or by class name?  Either is fine.  (I was assuming dotted path.)

> Tools/Scripts/webkitpy/xcode/simulated_device.py:30
> +from webkitpy.common.timeout_context import Timeout
>  from webkitpy.common.system.executive import ScriptError
>  from webkitpy.xcode.simulator import Simulator
>  from webkitpy.common.host import Host

Nit:  Alphabetized by neither dotted path nor class name?

[Jonathan Bedard]

(In reply to David Kilzer (:ddkilzer) from comment #4)
> Comment on attachment 317715 [details]
> Patch
> 
> > Tools/Scripts/webkitpy/common/timeout_context_unittest.py:36
> > +    def test_current_timeout(self):
> > +        self.assertEqual(None, Timeout.current())
> > +        with Timeout(1) as tmp:
> > +            self.assertEqual(tmp.data, Timeout.current())
> > +        self.assertEqual(None, Timeout.current())
> 
> This could be racy if the with: block doesn't execute in less than 1 second,
> correct?
> 

Yes. If the with: block doesn't execute in less than 1 second, this block of code would raise the default exception.

[Jonathan Bedard]

Created attachment 317741 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 317741 [details]
Patch for landing

Clearing flags on attachment: 317741

Committed r220483: <http://trac.webkit.org/changeset/220483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.