Bug 175390 - Allow nested timeouts in webkitpy
Summary: Allow nested timeouts in webkitpy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-09 09:53 PDT by Jonathan Bedard
Modified: 2017-08-09 14:58 PDT (History)
8 users (show)

See Also:


Attachments
Patch (21.94 KB, patch)
2017-08-09 10:00 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (21.99 KB, patch)
2017-08-09 14:15 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-08-09 09:53:12 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-08-09 09:53:42 PDT
<rdar://problem/33803003>
Comment 2 Jonathan Bedard 2017-08-09 10:00:00 PDT
Created attachment 317715 [details]
Patch
Comment 3 Build Bot 2017-08-09 10:02:31 PDT
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.
Comment 4 David Kilzer (:ddkilzer) 2017-08-09 13:26:52 PDT
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?
Comment 5 Jonathan Bedard 2017-08-09 14:08:10 PDT
(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.
Comment 6 Jonathan Bedard 2017-08-09 14:15:53 PDT
Created attachment 317741 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2017-08-09 14:58:21 PDT
Comment on attachment 317741 [details]
Patch for landing

Clearing flags on attachment: 317741

Committed r220483: <http://trac.webkit.org/changeset/220483>
Comment 8 WebKit Commit Bot 2017-08-09 14:58:24 PDT
All reviewed patches have been landed.  Closing bug.