Bug 98156 – XHR2 timeout property should allow late updates

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dominik Röttsches (drott)]

The timeout property should be implemented in a way that allows late updates, e.g.

* Timeout disabled after initially set
* Setting a timeout later to override and expected early load after a delay
* Timeout enabled after initially disabled
* Timeout set to expired value before load fires

I'd like to handle this separately as it requires somewhat bigger refactoring.

[Alexey Proskuryakov]

Does this require ability to change the timeout of an already running request? If so, is that even possible with existing network back-ends?

[Dominik Röttsches (drott)]

(In reply to comment #1)
> Does this require ability to change the timeout of an already running request? If so, is that even possible with existing network back-ends?

Yes, assuming no additional ResourceRequest API changes, the semantics of setTimeoutInterval would have to be defined as "at any time, overriding the previous value and updating the timer". NSUrlConnection does probably not support that - I am not sure. For the soup and Qt backends, it could be implemented.

[youenn fablet]

Why not handling timeout for async requests directly within WebCore?
That would make it simple to handle late updates. 
It would also make this feature consistent across all WebKit platforms.

XMLHttpRequest seems a natural choice but I would tend to go with ThreadableLoader to keep the possibility for other users of ThreadableLoader to use async timeout.

[Alexey Proskuryakov]

An implementation at a high level would have the disadvantage of not knowing about buffering at lower levels.

[youenn fablet]

(In reply to comment #4)
> An implementation at a high level would have the disadvantage of not knowing about buffering at lower levels.

The lower level approach requires adding some plumbery from ThreadableLoader all the way down to ResourceHandle. 
That also requires adding code for each low level HTTP back-end (soup, cf, curl).

I wonder whether this is worth the extra complexity.
Can you elaborate a bit on the expected benefits?

[youenn fablet]

(In reply to comment #1)
> Does this require ability to change the timeout of an already running
> request? If so, is that even possible with existing network back-ends?

https://xhr.spec.whatwg.org/#the-send%28%29-method gives additional information about timeout handling.
Basically, when timeout kicks in, XHR is responsible to abort the underlying fetch call. This removes the need for network backends to support timeout.

That makes it easier to support timeout value update or when the same resource is requested concurrently with different timeouts.

[Alexey Proskuryakov]

One implementation complexity is that the timeout should use monotonic time, not wall time.

[youenn fablet]

Created attachment 260577 [details]
Patch

[youenn fablet]

(In reply to comment #8)
> Created attachment 260577 [details]
> Patch

I am wondering whether ENABLE(XHR_TIMEOUT) compilation guard could be removed.

[Alexey Proskuryakov]

Comment on attachment 260577 [details]
Patch

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

> I am wondering whether ENABLE(XHR_TIMEOUT) compilation guard could be removed.

Yes.

> Source/WebCore/xml/XMLHttpRequest.cpp:139
> +    , m_timeoutTimer(*this, &XMLHttpRequest::didTimeout)

A pre-existing issue: the function should be named didTimeOut, because the phrase is "did time out", not "did timeout".

> Source/WebCore/xml/XMLHttpRequest.cpp:289
> +    m_timeoutTimer.startOneShot(std::max(0.0, (m_timeoutMilliseconds / 1000) - (monotonicallyIncreasingTime() - m_sendingTime)));

(m_timeoutMilliseconds / 1000) will be zero when the timeout is set to 500 ms. This is presumably not the right behavior.

> Source/WebCore/xml/XMLHttpRequest.h:263
> +    Timer m_timeoutTimer;

How did you confirm that this timer is based off monotonic time? I couldn't easily find that in CFRunLoopTimer documentation.

[Darin Adler]

Comment on attachment 260577 [details]
Patch

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

New code should use std::chrono types and functions rather than "double" for times.

>> Source/WebCore/xml/XMLHttpRequest.cpp:289
>> +    m_timeoutTimer.startOneShot(std::max(0.0, (m_timeoutMilliseconds / 1000) - (monotonicallyIncreasingTime() - m_sendingTime)));
> 
> (m_timeoutMilliseconds / 1000) will be zero when the timeout is set to 500 ms. This is presumably not the right behavior.

A simple way to mitigate that is to write / 1000.0 as we do below. Might even want a helper function for that.

With std::chrono it would be something like:

    m_timeoutTimer.startOneShot(std::max(0, std::chrono::milliseconds{m_timeoutMilliseconds} - (std::chrono::steady_clock::now() - m_sendingTime)));

Not sure how to get a 0 of the correct type.

> Source/WebCore/xml/XMLHttpRequest.cpp:779
> +            m_sendingTime = monotonicallyIncreasingTime();

In new code I believe we’d prefer using std::chrono::steady_clock rather than monotonicallyIncreasingTime.

> Source/WebCore/xml/XMLHttpRequest.cpp:780
> +            m_timeoutTimer.startOneShot(m_timeoutMilliseconds / 1000.0);

Using std::chrono I think we can write:

    m_timeoutTimer.startOneShot(std::chrono::milliseconds{m_timeoutMilliseconds});

> Source/WebCore/xml/XMLHttpRequest.cpp:851
> +    if (m_timeoutTimer.isActive())
> +        m_timeoutTimer.stop();

I believe an unconditional stop should be efficient enough without an explicit isActive check.

> Source/WebCore/xml/XMLHttpRequest.cpp:1141
> +    if (m_timeoutTimer.isActive())
> +        m_timeoutTimer.stop();

Ditto.

> Source/WebCore/xml/XMLHttpRequest.h:261
> +    unsigned long m_timeoutMilliseconds { 0 };

The use of the type unsigned long here is not new, but it’s peculiar. If we want a 32-bit integer, that would be unsigned, not long. Perhaps someone was confused by IDL, since "unsigned long" in our IDL is how we write the thing we call "unsigned" in our C++ code.

> Source/WebCore/xml/XMLHttpRequest.h:262
> +    double m_sendingTime { 0 };

I’d prefer to see this be:

    std::chrono::steady_clock::time_point m_sendingTime;

>> Source/WebCore/xml/XMLHttpRequest.h:263
>> +    Timer m_timeoutTimer;
> 
> How did you confirm that this timer is based off monotonic time? I couldn't easily find that in CFRunLoopTimer documentation.

I am not sure that matters. The code only subtracts values of monotonicallyIncreasingTime from other values of monotonicallyIncreasingTime, so it doesn’t depend directly on what the Timer timebase is. Maybe I’m missing something.

[Alexey Proskuryakov]

> I am not sure that matters. The code only subtracts values of monotonicallyIncreasingTime from other values of monotonicallyIncreasingTime, so it doesn’t depend directly on what the Timer timebase is. Maybe I’m missing something.

When the code calls Timer.startOneShot(delta), does this actually mean that the timer will fire after delta milliseconds, or that it will set a timer at a specific wall time (current plus delta), which can result at the interval ending up different when the wall time is adjusted?

Maybe it's a silly question, and the answer is that it's obviously the former. The reason why I'm asking is that all the code below Timer (including CFRunLoop) operates on absolute times, and I couldn't find it documented anywhere that it's not subject to adjustment.

[Darin Adler]

(In reply to comment #12)
> When the code calls Timer.startOneShot(delta), does this actually mean that
> the timer will fire after delta milliseconds, or that it will set a timer at
> a specific wall time (current plus delta), which can result at the interval
> ending up different when the wall time is adjusted?

Obviously. we want the former. Not sure how Timer accomplishes this.

> The reason why I'm asking is that all the code below Timer
> (including CFRunLoop) operates on absolute times, and I couldn't find it
> documented anywhere that it's not subject to adjustment.

This may well be something we need to fix in Timer. When working on higher levels I think we should program as if it’s true and dig into how to make the Timer implementation do this properly. Anders might have some ideas.

[Anders Carlsson]

(In reply to comment #13)
> (In reply to comment #12)
> > When the code calls Timer.startOneShot(delta), does this actually mean that
> > the timer will fire after delta milliseconds, or that it will set a timer at
> > a specific wall time (current plus delta), which can result at the interval
> > ending up different when the wall time is adjusted?
> 
> Obviously. we want the former. Not sure how Timer accomplishes this.
> 
> > The reason why I'm asking is that all the code below Timer
> > (including CFRunLoop) operates on absolute times, and I couldn't find it
> > documented anywhere that it's not subject to adjustment.
> 
> This may well be something we need to fix in Timer. When working on higher
> levels I think we should program as if it’s true and dig into how to make
> the Timer implementation do this properly. Anders might have some ideas.

CFRunLoopTimer uses mach_absolute_time which is not related to wall-clock time.

[youenn fablet]

(In reply to comment #13)
> (In reply to comment #12)
> > When the code calls Timer.startOneShot(delta), does this actually mean that
> > the timer will fire after delta milliseconds, or that it will set a timer at
> > a specific wall time (current plus delta), which can result at the interval
> > ending up different when the wall time is adjusted?
> 
> Obviously. we want the former. Not sure how Timer accomplishes this.
> 
> > The reason why I'm asking is that all the code below Timer
> > (including CFRunLoop) operates on absolute times, and I couldn't find it
> > documented anywhere that it's not subject to adjustment.
> 
> This may well be something we need to fix in Timer. When working on higher
> levels I think we should program as if it’s true and dig into how to make
> the Timer implementation do this properly. Anders might have some ideas.

I took a look at Timer and SharedTimer before writing the patch.
They both consistently use and require to work with monotonic time.

Doing some more checks, Mac SharedTimer is converting duration to Wall time just before using CF timer.
WorkerSharedTimer on the contrary is working with wall time, since the lower WTF code expects absolute time.

Also, Timer is already in use to throttle XHR events, apparently without any issue.

[youenn fablet]

Created attachment 260710 [details]
Patch for landing

[youenn fablet]

Thanks for the reviews.
I tried to accommodate the patch with most of your the feedback.
See below for more details.

(In reply to comment #10)
> Comment on attachment 260577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260577&action=review
> 
> > I am wondering whether ENABLE(XHR_TIMEOUT) compilation guard could be removed.
> 
> Yes.

I'll try to prepare a patch to remove it.

> > Source/WebCore/xml/XMLHttpRequest.cpp:139
> > +    , m_timeoutTimer(*this, &XMLHttpRequest::didTimeout)
> 
> A pre-existing issue: the function should be named didTimeOut, because the
> phrase is "did time out", not "did timeout".

I am a bit uncertain here.
We use "timeout..." variable names which are ok like this.
Other APIs like ResourceError::isTimeout() would need to be changed for consistency.
I did not do the change in this patch but can look at changing that in the future if that is the consensus.

> > Source/WebCore/xml/XMLHttpRequest.cpp:289
> > +    m_timeoutTimer.startOneShot(std::max(0.0, (m_timeoutMilliseconds / 1000) - (monotonicallyIncreasingTime() - m_sendingTime)));
> 
> (m_timeoutMilliseconds / 1000) will be zero when the timeout is set to 500
> ms. This is presumably not the right behavior.

Thanks for spotting it.
Fixed using std::chrono as suggested by Darin.

> >> Source/WebCore/xml/XMLHttpRequest.cpp:289
> >> +    m_timeoutTimer.startOneShot(std::max(0.0, (m_timeoutMilliseconds / 1000) - (monotonicallyIncreasingTime() - m_sendingTime)));
> > 
> > (m_timeoutMilliseconds / 1000) will be zero when the timeout is set to 500 ms. This is presumably not the right behavior.
> 
> A simple way to mitigate that is to write / 1000.0 as we do below. Might
> even want a helper function for that.
> 
> With std::chrono it would be something like:
> 
>     m_timeoutTimer.startOneShot(std::max(0,
> std::chrono::milliseconds{m_timeoutMilliseconds} -
> (std::chrono::steady_clock::now() - m_sendingTime)));
> 
> Not sure how to get a 0 of the correct type.

I used std::chrono::duration<double>::count() to have a double at the end.

> > Source/WebCore/xml/XMLHttpRequest.cpp:779
> > +            m_sendingTime = monotonicallyIncreasingTime();
> 
> In new code I believe we’d prefer using std::chrono::steady_clock rather
> than monotonicallyIncreasingTime.
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:780
> > +            m_timeoutTimer.startOneShot(m_timeoutMilliseconds / 1000.0);
> 
> Using std::chrono I think we can write:
> 
>    
> m_timeoutTimer.startOneShot(std::chrono::milliseconds{m_timeoutMilliseconds});

OK

> > Source/WebCore/xml/XMLHttpRequest.cpp:851
> > +    if (m_timeoutTimer.isActive())
> > +        m_timeoutTimer.stop();
> 
> I believe an unconditional stop should be efficient enough without an
> explicit isActive check.

I am not sure of this.
isActive is inlined and is then just a boolean check.
Timer::stop does several assignments, calls subroutines...
I kept the isActive() check in the patch.

> > Source/WebCore/xml/XMLHttpRequest.h:261
> > +    unsigned long m_timeoutMilliseconds { 0 };
> 
> The use of the type unsigned long here is not new, but it’s peculiar. If we
> want a 32-bit integer, that would be unsigned, not long. Perhaps someone was
> confused by IDL, since "unsigned long" in our IDL is how we write the thing
> we call "unsigned" in our C++ code.

OK, fixed the getter and setter as well.

> > Source/WebCore/xml/XMLHttpRequest.h:262
> > +    double m_sendingTime { 0 };
> 
> I’d prefer to see this be:
> 
>     std::chrono::steady_clock::time_point m_sendingTime;

OK

[youenn fablet]

> Doing some more checks, Mac SharedTimer is converting duration to Wall time
> just before using CF timer.
> WorkerSharedTimer on the contrary is working with wall time, since the lower
> WTF code expects absolute time.
> 
> Also, Timer is already in use to throttle XHR events, apparently without any
> issue.

What I wrote is at best unclear, here is a hopefully better version.

Mac SharedTimer is converting duration to CF absolute time just before using CF timer.
WorkerSharedTimer is working with Wall time, the lower WTF API (MessageQueue/Condition) expecting it. It is then converted back to an interval. I guess it would be good to change these to monotonic time for consistency.

[WebKit Commit Bot]

Comment on attachment 260710 [details]
Patch for landing

Clearing flags on attachment: 260710

Committed r189445: <http://trac.webkit.org/changeset/189445>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 260577 [details]
Patch

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

>>> Source/WebCore/xml/XMLHttpRequest.cpp:139
>>> +    , m_timeoutTimer(*this, &XMLHttpRequest::didTimeout)
>> 
>> A pre-existing issue: the function should be named didTimeOut, because the phrase is "did time out", not "did timeout".
> 
> I am a bit uncertain here.
> We use "timeout..." variable names which are ok like this.
> Other APIs like ResourceError::isTimeout() would need to be changed for consistency.
> I did not do the change in this patch but can look at changing that in the future if that is the consensus.

Alexey is right.

“This error is a timeout.” is acceptable. The word "timeout" is a noun.
“The request did time out.” is too. The phrase "time out" is a verb phrase
“The request did timeout.” is not. The word "timeout" is not a verb.

So we could have didTimeOut as well as isTimeout. Of we could have didEncounterTimeout or something like that.

[youenn fablet]

(In reply to comment #21)
> Comment on attachment 260577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260577&action=review
> 
> >>> Source/WebCore/xml/XMLHttpRequest.cpp:139
> >>> +    , m_timeoutTimer(*this, &XMLHttpRequest::didTimeout)
> >> 
> >> A pre-existing issue: the function should be named didTimeOut, because the phrase is "did time out", not "did timeout".
> > 
> > I am a bit uncertain here.
> > We use "timeout..." variable names which are ok like this.
> > Other APIs like ResourceError::isTimeout() would need to be changed for consistency.
> > I did not do the change in this patch but can look at changing that in the future if that is the consensus.
> 
> Alexey is right.
> 
> “This error is a timeout.” is acceptable. The word "timeout" is a noun.
> “The request did time out.” is too. The phrase "time out" is a verb phrase
> “The request did timeout.” is not. The word "timeout" is not a verb.
> 
> So we could have didTimeOut as well as isTimeout. Of we could have
> didEncounterTimeout or something like that.

didReachTimeout seems good to me then.
I will change this when removing the XHR_TIMEOUT guard.

[Alexey Proskuryakov]

Looks like this made a test flaky: http/tests/xmlhttprequest/ontimeout-response-getters.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=xmlhttprequest%2Fontimeout-response-getters

Youenn, could you please look int that? This is making the dashboard pretty red.

[Alexey Proskuryakov]

Comment on attachment 260710 [details]
Patch for landing

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

> Source/WebCore/xml/XMLHttpRequest.cpp:-771
> -    if (m_timeoutMilliseconds)
> -        request.setTimeoutInterval(m_timeoutMilliseconds / 1000.0);

Coming back to this, I don't understand this change.

Doesn't this result in network timeout happening as per default? I think that you need to set it to infinity.

[youenn fablet]

(In reply to comment #24)
> Comment on attachment 260710 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260710&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:-771
> > -    if (m_timeoutMilliseconds)
> > -        request.setTimeoutInterval(m_timeoutMilliseconds / 1000.0);
> 
> Coming back to this, I don't understand this change.
> 
> Doesn't this result in network timeout happening as per default? I think
> that you need to set it to infinity.

With the previous version of the code, if a user was not setting a timeout (or was setting it to zero), the default network timeout would kick in.

With the current code, the same is happening, especially if a user sets it to a value and then to zero after the request is sent.

Setting the network timeout value explicitly to a sufficiently large value might make XHR more consistent across network backends.

[Alexey Proskuryakov]

No, this is not what I'm saying.

With the current code, default network timeout will kick in before the one specified as xhr.timeout if the default one is shorter. This is a regression from this patch.

[youenn fablet]

> With the current code, default network timeout will kick in before the one
> specified as xhr.timeout if the default one is shorter.

Hence my initial suggestion to always set a network timeout to a sufficiently large value.

It is not totally clear to me what the spec mandates when timeout is not set.
It might make sense to change 0 value semantics from 'default' to 'infinite' in XHR and the network backends.

[Alexey Proskuryakov]

> Hence my initial suggestion to always set a network timeout to a sufficiently large value.

The default timeout is exposed as API, it's up to the client to decide what it is. If the client didn't decide, then using the platform default makes sense in my opinion.

WebKit can (and should) disable the network timeout only when using a timer to implement timeout on its own.

[youenn fablet]

(In reply to comment #28)
> > Hence my initial suggestion to always set a network timeout to a sufficiently large value.
> 
> The default timeout is exposed as API, it's up to the client to decide what
> it is. If the client didn't decide, then using the platform default makes
> sense in my opinion.
> 
> WebKit can (and should) disable the network timeout only when using a timer
> to implement timeout on its own.

I filed bug 149704 to keep track of this.