Bug 215584 – [webkitcorepy] Move Timeout to webkitcorepy

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

[Jonathan Bedard]

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.

[Radar WebKit Bug Importer]

<rdar://problem/67270713>

[Jonathan Bedard]

Created attachment 406744 [details]
Patch

[Jonathan Bedard]

(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):
        ....
```

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 406793 [details]
Patch

[Jonathan Bedard]

Created attachment 406883 [details]
Patch

[Jonathan Bedard]

Created attachment 406928 [details]
Patch for landing

[EWS]

Committed r265942: <https://trac.webkit.org/changeset/265942>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406928 [details].

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 406945 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #11)
> Created attachment 406945 [details]
> Patch

(Will be waiting to land this until next week)

[Aakash Jain]

(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

[Jonathan Bedard]

(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.

[Jonathan Bedard]

Created attachment 407107 [details]
Patch

[Jonathan Bedard]

Created attachment 407113 [details]
Patch

[Jonathan Bedard]

Created attachment 407122 [details]
Patch

[Jonathan Bedard]

Created attachment 407130 [details]
Patch

[Jonathan Bedard]

Created attachment 407140 [details]
Patch

[Jonathan Bedard]

Created attachment 407196 [details]
Patch

[Jonathan Bedard]

Created attachment 407205 [details]
Patch

[Jonathan Bedard]

Created attachment 407249 [details]
Patch

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 407305 [details]
Patch

[Jonathan Bedard]

Created attachment 407374 [details]
Patch

[EWS]

Committed r266224: <https://trac.webkit.org/changeset/266224>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407374 [details].

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 407396 [details]
Patch

[EWS]

Committed r266277: <https://trac.webkit.org/changeset/266277>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407396 [details].