WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
101731
[Qt] Support ResourceRequest's setTimeoutInterval
https://bugs.webkit.org/show_bug.cgi?id=101731
Summary
[Qt] Support ResourceRequest's setTimeoutInterval
Allan Sandfeld Jensen
Reported
2012-11-09 02:11:58 PST
The Qt port currently does not support timeoutInterval on ResourceRequest, for more information see
bug #98397
and
bug #74802
Attachments
Patch
(10.33 KB, patch)
2012-11-09 02:18 PST
,
Allan Sandfeld Jensen
no flags
Details
Formatted Diff
Diff
Patch
(9.96 KB, patch)
2012-11-09 04:51 PST
,
Allan Sandfeld Jensen
no flags
Details
Formatted Diff
Diff
Patch
(9.72 KB, patch)
2012-11-09 04:54 PST
,
Allan Sandfeld Jensen
no flags
Details
Formatted Diff
Diff
Patch
(9.04 KB, patch)
2012-11-12 02:30 PST
,
Allan Sandfeld Jensen
hausmann
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Allan Sandfeld Jensen
Comment 1
2012-11-09 02:18:49 PST
Created
attachment 173246
[details]
Patch
Jocelyn Turcotte
Comment 2
2012-11-09 03:39:45 PST
Comment on
attachment 173246
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=173246&action=review
> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:43 > +static const int gTimeoutError = -1001;
I'm not sure if that's necessary, XMLHttpRequest::didFail relies on ResourceError::isTimeout(), and this error code is converted only in ResourceErrorMac.mm which we don't use. I'm also wondering if we should setIsTimeout(true) for QNetworkReply::TimeoutError errors in QNetworkReplyHandler::errorForReply. That's why this value seemed to be necessary on the Mac port.
> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:357 > + m_timeoutTimer.setSingleShot(true); > + m_timeoutTimer.start(ms);
Should we support the note there?
http://www.w3.org/TR/XMLHttpRequest/#dom-xmlhttprequest-timeout
"Note: This implies that the timeout attribute can be set while fetching is in progress. If that occurs it will still be measured relative to the start of fetching."
> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:368 > +void QNetworkReplyWrapper::didTimeout() > +{ > + QueueLocker lock(m_queue); > + m_timeoutError = true; > + resetConnections(); > + setFinished(); > + m_queue->push(&QNetworkReplyHandler::finish); > +}
Why not put the timer in QNetworkReplyWrapper instead?
Allan Sandfeld Jensen
Comment 3
2012-11-09 04:47:11 PST
(In reply to
comment #2
)
> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:357 > > + m_timeoutTimer.setSingleShot(true); > > + m_timeoutTimer.start(ms); > > Should we support the note there?
http://www.w3.org/TR/XMLHttpRequest/#dom-xmlhttprequest-timeout
> "Note: This implies that the timeout attribute can be set while fetching is in progress. If that occurs it will still be measured relative to the start of fetching." >
We currently do not support updating a ResourceRequest once it has been started. I think this would be implemented in ResourceRequest::doUpdatePlatformRequest, but we have an empty implementation of that. Also currently I don't think any code calls doUpdatePlatformRequest, it seems to be designed to be triggered by other platform code.
Allan Sandfeld Jensen
Comment 4
2012-11-09 04:51:14 PST
Created
attachment 173266
[details]
Patch Moved timer to QNetworkReplyHandler, and made the error more Qt standard, though it is not really used anywhere
Allan Sandfeld Jensen
Comment 5
2012-11-09 04:54:01 PST
Created
attachment 173267
[details]
Patch Removed left-over line in header
Allan Sandfeld Jensen
Comment 6
2012-11-09 05:20:11 PST
(In reply to
comment #3
)
> (In reply to
comment #2
) > > > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:357 > > > + m_timeoutTimer.setSingleShot(true); > > > + m_timeoutTimer.start(ms); > > > > Should we support the note there?
http://www.w3.org/TR/XMLHttpRequest/#dom-xmlhttprequest-timeout
> > "Note: This implies that the timeout attribute can be set while fetching is in progress. If that occurs it will still be measured relative to the start of fetching." > > > We currently do not support updating a ResourceRequest once it has been started. I think this would be implemented in ResourceRequest::doUpdatePlatformRequest, but we have an empty implementation of that. Also currently I don't think any code calls doUpdatePlatformRequest, it seems to be designed to be triggered by other platform code.
Btw, this is not currently possible anyway in XHR, see
bug #98156
Jocelyn Turcotte
Comment 7
2012-11-09 07:30:35 PST
Comment on
attachment 173267
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=173267&action=review
> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:479 > + , m_timeoutTimer(this)
I think it's better not to set the parent for inline members.
> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:575 > + if (m_replyWrapper->isFinished())
Just in case somebody forgets a m_timeoutTimer.stop() some time in the future, I would remove m_replyWrapper != 0 from the assert above, move the assert after the following return and also return early here if m_replyWrapper == 0. It's easy to forget and alsmost impossible to test every code path.
> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:584 > + ResourceError timeoutError("QtNetwork", QNetworkReply::TimeoutError, m_replyWrapper->reply()->url().toString(), "Request timed out");
I think the QNetworkReply::TimeoutError adds noise here, future readers might believe that it's useful.
> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:814 > + double timeoutInSeconds = d->m_firstRequest.timeoutInterval(); > + if (timeoutInSeconds > 0 && timeoutInSeconds < (INT_MAX / 1000)) > + m_timeoutTimer.start(timeoutInSeconds * 1000);
Just in case sendNetworkRequest completes the request synchronously (local files or resources in cache), it might be better to start the timer before so that finish() doesn't call stop before it's started.
Allan Sandfeld Jensen
Comment 8
2012-11-09 10:04:14 PST
(In reply to
comment #7
)
> (From update of
attachment 173267
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=173267&action=review
> > > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:479 > > + , m_timeoutTimer(this) > > I think it's better not to set the parent for inline members. >
Right.
> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:575 > > + if (m_replyWrapper->isFinished()) > > Just in case somebody forgets a m_timeoutTimer.stop() some time in the future, I would remove m_replyWrapper != 0 from the assert above, move the assert after the following return and also return early here if m_replyWrapper == 0. > It's easy to forget and alsmost impossible to test every code path. >
I think that it is easy to forget is a good reason to keep it as an assert (so that debug bots turn red), but you right, it would be good to have a safe return path for release builds as well. I will see if I can make it more error-safe.
> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:814 > > + double timeoutInSeconds = d->m_firstRequest.timeoutInterval(); > > + if (timeoutInSeconds > 0 && timeoutInSeconds < (INT_MAX / 1000)) > > + m_timeoutTimer.start(timeoutInSeconds * 1000); > > Just in case sendNetworkRequest completes the request synchronously (local files or resources in cache), it might be better to start the timer before so that finish() doesn't call stop before it's started.
A good thing about having it in start() is that start() is not called directly but inserted into the queue from QNetworkReplyWrapper and so are finish() calls, which should guarantee us that finish() will execute after start() . We could have a problem with abort() though, because abort() is called directly, I will need to take a second look at that case.
Dominik Röttsches (drott)
Comment 9
2012-11-11 04:02:50 PST
Nice to see another port supporting the feature!
Allan Sandfeld Jensen
Comment 10
2012-11-12 02:30:20 PST
Created
attachment 173594
[details]
Patch Protect timeout() better against misuse of API, and drop incorrect Timer initialization
Simon Hausmann
Comment 11
2012-11-12 06:13:05 PST
Comment on
attachment 173594
[details]
Patch r=me, but the timer should use QBasicTimer. Please fix before landing :)
Allan Sandfeld Jensen
Comment 12
2012-11-12 08:14:26 PST
Committed
r134243
: <
http://trac.webkit.org/changeset/134243
>
Csaba Osztrogonác
Comment 13
2012-11-13 07:16:53 PST
(In reply to
comment #12
)
> Committed
r134243
: <
http://trac.webkit.org/changeset/134243
>
These http/tests/xmlhttprequest/timeout/* tests are very very very sloooow. :-/ These tests made layout testing time from 20 mins to 24 mins ... It is nonsense if only a couple of tests takes the 17% of the runtime of 30.000 tests. Any idea how can we fix them?
Allan Sandfeld Jensen
Comment 14
2012-11-13 15:06:10 PST
(In reply to
comment #13
)
> (In reply to
comment #12
) > > Committed
r134243
: <
http://trac.webkit.org/changeset/134243
> > > These http/tests/xmlhttprequest/timeout/* tests are very very very sloooow. :-/ These tests made layout testing time from 20 mins to 24 mins ... It is nonsense if only a couple of tests takes the 17% of the runtime of 30.000 tests. > > Any idea how can we fix them?
Well, there is no reason the timeouts have to be 2 or 5 seconds. The files are local, so perhaps dividing the timeouts by 10 would work?
Dominik Röttsches (drott)
Comment 15
2012-11-13 23:17:35 PST
(In reply to
comment #14
)
> (In reply to
comment #13
) > > (In reply to
comment #12
) > > > Committed
r134243
: <
http://trac.webkit.org/changeset/134243
> > > > > These http/tests/xmlhttprequest/timeout/* tests are very very very sloooow. :-/ These tests made layout testing time from 20 mins to 24 mins ... It is nonsense if only a couple of tests takes the 17% of the runtime of 30.000 tests. > > > > Any idea how can we fix them? > > Well, there is no reason the timeouts have to be 2 or 5 seconds. The files are local, so perhaps dividing the timeouts by 10 would work?
I can look into tuning those values,
bug 102184
. Might take a few days though.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug