Webkitpy has a timeout class which uses alarms to raise an exception. This is a useful idea, although many functions which may block have their own native timeouts. To work around this, we should allow our timeout class to be disabled for a blocks that a program may use a more appropriate timeout for APIs which support it.
<rdar://problem/67270713>
Created attachment 406744 [details] Patch
(In reply to Jonathan Bedard from comment #2) > Created attachment 406744 [details] > Patch Before landing this, want to be clear that this isn't a 100% move, while the new class is a drop in replacement to the old one, they aren't identical.
Comment on attachment 406744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406744&action=review > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/timeout.py:36 > + _process_to_timeout_map = {} How about using `collections.defaultdict(list)` to avoid boundary check? > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/timeout.py:121 > + def sleep(cls, t): Nit: maybe call it 'seconds' to be clearer here. > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/timeout.py:161 > + for i in range(len(self._process_to_timeout_map[os.getpid()]) - 1): > + if self.data.alarm_time < self._process_to_timeout_map[os.getpid()][i + 1].alarm_time: > + self._process_to_timeout_map[os.getpid()].insert(i, self.data) > + break It looks like we are doing a linear search on an sorted array, we can just use `bisect.insort` https://docs.python.org/3/library/bisect.html > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/timeout.py:167 > + if current_timeout and current_timeout.alarm_time < self.data.alarm_time: > + for i in range(len(self._process_to_timeout_map[os.getpid()]) - 1): > + if self.data.alarm_time < self._process_to_timeout_map[os.getpid()][i + 1].alarm_time: > + self._process_to_timeout_map[os.getpid()].insert(i, self.data) > + break > + self._process_to_timeout_map[os.getpid()].append(self.data) > + > + # This is the most urgent timeout > + else: > + self._process_to_timeout_map[os.getpid()] = [self.data] + self._process_to_timeout_map.get(os.getpid(), []) > + This logic can be simplified to `bisect.insort(self._process_to_timeout_map[os.getpid()], self.data)` if we use defaultdict for self._process_to_timeout_map and implement `__cmp__` for Data class. For bisect.insort, you can refer to https://docs.python.org/3/library/bisect.html, it's basically upper_bound in cpp. > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/tests/timeout_unittest.py:31 > + Do we have a test case that an exception is raised in a Timeout context, and we expect the `_process_to_timeout_map` to be updated correctly? > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/tests/timeout_unittest.py:71 > + with mocks.Time: > + with OutputCapture() as capturer: > + with Timeout(1): > + Timeout.check() > + self.assertRaises(Timeout.Exception, time.sleep, 1) > + To avoid code getting too nested, I think we can just do ``` with mocks.Time, OutputCapture() as capturer, Timeout(1): ... ``` or ``` with mocks.Time, OutputCapture() as capturer: with Timeout(1): .... ```
Comment on attachment 406744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406744&action=review >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/tests/timeout_unittest.py:31 >> + > > Do we have a test case that an exception is raised in a Timeout context, and we expect the `_process_to_timeout_map` to be updated correctly? This is a good point. I've added a test that covers the "what happens when an exception has already been triggered and caught" case.
Created attachment 406793 [details] Patch
Created attachment 406883 [details] Patch
Created attachment 406928 [details] Patch for landing
Committed r265942: <https://trac.webkit.org/changeset/265942> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406928 [details].
Reopening to attach new patch.
Created attachment 406945 [details] Patch
(In reply to Jonathan Bedard from comment #11) > Created attachment 406945 [details] > Patch (Will be waiting to land this until next week)
(In reply to EWS from comment #9) > Committed r265942: <https://trac.webkit.org/changeset/265942> For some reason, this seems to have broke ios-wk2 tests, see Bug 215742
(In reply to Aakash Jain from comment #13) > (In reply to EWS from comment #9) > > Committed r265942: <https://trac.webkit.org/changeset/265942> > For some reason, this seems to have broke ios-wk2 tests, see Bug 215742 I’ll be revisiting this Monday. This is ultimately about the way that python’s multiprocess does forking, will take quite a bit of trial and error to track down.
Created attachment 407107 [details] Patch
Created attachment 407113 [details] Patch
Created attachment 407122 [details] Patch
Created attachment 407130 [details] Patch
Created attachment 407140 [details] Patch
Created attachment 407196 [details] Patch
Created attachment 407205 [details] Patch
Created attachment 407249 [details] Patch
Comment on attachment 407249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407249&action=review > Tools/Scripts/webkitpy/xcode/simulated_device.py:642 > + with Timeout(timeout, handler=RuntimeError(u'Timed out waiting for process to open {} on {}'.format(bundle_id, self.udid)), patch=False): Took quite a bit of trial and error, but I found the original issue with the change, apparent mock.patch breaks future forking on MacOS, which is super surprising. The ultimate solution will be fixing our multiprocessing code, but that's not a change for this patch. All we need to do is opt out of patching time.sleep when booting simulators.
Created attachment 407305 [details] Patch
Created attachment 407374 [details] Patch
Committed r266224: <https://trac.webkit.org/changeset/266224> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407374 [details].
Created attachment 407396 [details] Patch
Committed r266277: <https://trac.webkit.org/changeset/266277> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407396 [details].