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.
<rdar://problem/33803003>
Created attachment 317715 [details] Patch
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 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?
(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.
Created attachment 317741 [details] Patch for landing
Comment on attachment 317741 [details] Patch for landing Clearing flags on attachment: 317741 Committed r220483: <http://trac.webkit.org/changeset/220483>
All reviewed patches have been landed. Closing bug.