Bug 222305 – Timeout calculations are error-prone for compound IPC operations

• Component: WebKit2
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

Timeout calculations are error-prone for compound IPC operations

Consider a use-case where a top-level IPC operation has a logical timeout value.
If this operation is implemented as composition of sends, waits and semaphore waits, then it's cumbersome to pass correct timeout values for each of the calls.

Clients should be able to pass timeout in seconds relative to current time (possible today)
Clients should be able to pass timeout as absolute point of time (not possible today)

[Kimmo Kinnunen]

Created attachment 421299 [details]
Patch

[Kimmo Kinnunen]

Created attachment 421336 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/74737421>

[Geoffrey Garen]

Comment on attachment 421336 [details]
Patch

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

r=me

> Source/WebKit/Platform/IPC/Timeout.h:47
> +    explicit constexpr Timeout(MonotonicTime absoluteTimeout)
> +        : m_absoluteTimeout(absoluteTimeout)
> +    {
> +    }

Is this constructor ever used? I looked but didn't see a use. It feels a little weird to construct a timeout from an absolute time, since a timeout is a time point relative to a starting point; maybe we should not encourage it.

> Source/WebKit/Platform/IPC/Timeout.h:48
> +    static constexpr Timeout infinity() { return { }; }

I think we should either make the default constructor an infinite timeout, or remove the default constructor and use infinity() everywhere. Having two ways to make an infinite timeout will produce confusing code in the long run. It looks like you preferred infinity() in this patch, so why don't we remove the default constructor.

> Source/WebKit/Platform/IPC/Timeout.h:50
> +    operator Seconds() const { return std::max((m_absoluteTimeout - MonotonicTime::now()), 0_s ); }

Let's remove this operator, or give it an explicit name like "remainingSeconds()" or "secondsUntilDeadline()".

When an automatic type conversion also changes the meaning of the data, it reduces type safety.

In this case, the change in meaning is also ambiguous because a timeout could tell you (a) how many seconds it was originally scheduled for (b) how many seconds it has left or (c) a relative to the epoch or (d) b relative to the epoch.

> Source/WebKit/Platform/IPC/Timeout.h:52
> +    constexpr operator MonotonicTime() const { return m_absoluteTimeout; }
> +    operator TimeWithDynamicClockType() const { return m_absoluteTimeout; }

Can we replace these type conversions with a "MonotonicTime deadline() const { return m_abosluteTimeout; }"? Again, better to be explicit about what time value we're retrieving.

> Source/WebKit/Platform/IPC/Timeout.h:53
> +    bool hasPassed() const { return MonotonicTime::now() >= m_absoluteTimeout; }

I would call this didTimeOut().

> Source/WebKit/Platform/IPC/Timeout.h:55
> +    MonotonicTime m_absoluteTimeout;

I would call this m_deadline.

[Kimmo Kinnunen]

Created attachment 421799 [details]
Patch

[EWS]

Committed r273646: <https://commits.webkit.org/r273646>

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