Bug 94796 – [soup] Obey setTimeoutInterval in soup backend

• Component: Platform
• Status: RESOLVED FIXED

[Dominik Röttsches (drott)]

ResourceRequestBase has setTimeoutInterval which sets a network connection timeout. The soup backend should support timing out depending on this value.

[Dominik Röttsches (drott)]

Dan, any ideas/suggestions how to implement a timeout on a ResourceRequest basis? SOUP_SESSION_TIMEOUT seems to be the same value for the whole session. Can it be used for that purpose?

[Dan Winship]

no, you'd need to add new API (eg, soup_message_set_timeout())

[Dominik Röttsches (drott)]

Filed libsoup bug https://bugzilla.gnome.org/show_bug.cgi?id=682804

[Dominik Röttsches (drott)]

(In reply to comment #3)
> Filed libsoup bug https://bugzilla.gnome.org/show_bug.cgi?id=682804

After discussion in that bug, I implemented per message timeouts in ResourceHandleSoup in WebKit. Will upload my patch once bug 95755 is closed.

[Dominik Röttsches (drott)]

Created attachment 162224 [details]
setTimeoutInterval support for soup.

[Martin Robinson]

Comment on attachment 162224 [details]
setTimeoutInterval support for soup.

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

Looks okay to me, but I'd prefer to have Dan and/or Sergio look at this as well. r- because of a few small issues.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:75
> +// Use the same value as in NSURLError.h
> +#define TIMEOUT_ERROR -1001

For these type of values you should use static const int.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:772
> +        if (d->m_firstRequest.timeoutInterval() > 0)
> +            d->m_timeoutId = g_timeout_add(d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle);

Does g_timeout_add attach the source to the default context or to the current thread default context? Synchronous XMLHttpRequests push a new thread default context, so this might not fire for them.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:994
> +static int requestTimeoutCallback(gpointer data)

Callbacks should return gboolean.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1007
> +    return false;

You should use FALSE here.

[Dominik Röttsches (drott)]

Created attachment 162336 [details]
setTimeoutInterval support for soup, v2.

[Dominik Röttsches (drott)]

Created attachment 162352 [details]
setTimeoutInterval support for soup, v3.

[Dominik Röttsches (drott)]

Thanks for your review, Martin.
Dan, Sergio - could you take a look and perhaps give your informal r+? This approach should fit what we discussed earlier, Dan.

(In reply to comment #6)
> (From update of attachment 162224 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162224&action=review

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:75
> > +// Use the same value as in NSURLError.h
> > +#define TIMEOUT_ERROR -1001
> 
> For these type of values you should use static const int.

Changed to gTimeoutError as static const int. Hope the naming is correct.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:772
> > +        if (d->m_firstRequest.timeoutInterval() > 0)
> > +            d->m_timeoutId = g_timeout_add(d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle);
> 
> Does g_timeout_add attach the source to the default context or to the current thread default context? Synchronous XMLHttpRequests push a new thread default context, so this might not fire for them.

Good point - I thought I had checked that, but it was the default context, not the thread default. Fixed that by using the soup function for that purpose, and the thread default. Then, we need to store a handle to the timeout GSource, not a timeout id anymore.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:994
> > +static int requestTimeoutCallback(gpointer data)
> 
> Callbacks should return gboolean.

Fixed.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1007
> > +    return false;
> 
> You should use FALSE here.

Fixed.

[Sergio Villar Senin]

Comment on attachment 162352 [details]
setTimeoutInterval support for soup, v3.

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

> Source/WebCore/platform/network/ResourceHandleInternal.h:191
> +        GSource* m_timeoutSource;

I think you should use GOwnPtr for this. You'd have to add a free function to GOwnPtr.h for GSources. Also you'd have to use set/get to use it.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1040
> +            d->m_timeoutSource = soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle);

If I understood correctly the timeout only makes sense for network connections. If so, then we don't need it for non HTTP requests.

[Dominik Röttsches (drott)]

Created attachment 162467 [details]
setTimeoutInterval support for soup, v4.

[Dominik Röttsches (drott)]

Thanks for taking a look, Sergio.

(In reply to comment #10)
> (From update of attachment 162352 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162352&action=review
> 
> > Source/WebCore/platform/network/ResourceHandleInternal.h:191
> > +        GSource* m_timeoutSource;
> 
> I think you should use GOwnPtr for this. You'd have to add a free function to GOwnPtr.h for GSources. Also you'd have to use set/get to use it.

Changed to use GOwnPtr<GSource>.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1040
> > +            d->m_timeoutSource = soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle);
> 
> If I understood correctly the timeout only makes sense for network connections. If so, then we don't need it for non HTTP requests.

Since the timeout interval can be specified in milliseconds, there might be cases where for example a network mounted local filesystem hits a timeout threshold. So I think it's fine to keep it for local requests - at least the spec doesn't seem to rule that out.

[Dan Winship]

Comment on attachment 162467 [details]
setTimeoutInterval support for soup, v4.

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

> Source/WebCore/platform/network/ResourceHandleInternal.h:191
> +        GOwnPtr<GSource> m_timeoutSource;

GSources are refcounted. You should use GRefPtr (which in fact already supports GSource).

(Note though that when the GRefPtr releases the source, it doesn't call g_source_destroy(), so you'll need to do that explicitly before clearing it in cleanupSoupRequestOperation and sendRequestCallback.)

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:438
> +    d->m_timeoutSource.clear();

Is this the right place to clear the timeout? We haven't received the response body yet at this point.

(If you want to clear the timeout after receiving the entire response, then you can just remove this line and not add another, since closeCallback() calls cleanupSoupRequestOperation() and so will hit the clear() there.)

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:767
> +            d->m_timeoutSource.set(soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle));

soup_add_timeout() and the related functions are broken in that they return a GSource that you don't own a reference on. As long as you aren't using multiple threads (which we aren't), then this won't cause problems, but when you switch to GRefPtr, make sure you *don't* use adoptGRef()

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:868
> +        if (d->m_firstRequest.timeoutInterval() > 0)
> +            d->m_timeoutSource.set(soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, this));

should it cancel the timeout when defersLoading is true?

[Dominik Röttsches (drott)]

Created attachment 162754 [details]
setTimeoutInterval support for soup, v5.

[Dominik Röttsches (drott)]

Dan, thanks for your review.

(In reply to comment #13)
> (From update of attachment 162467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162467&action=review
> 
> > Source/WebCore/platform/network/ResourceHandleInternal.h:191
> > +        GOwnPtr<GSource> m_timeoutSource;
> 
> GSources are refcounted. You should use GRefPtr (which in fact already supports GSource).

Changed, thanks.

> (Note though that when the GRefPtr releases the source, it doesn't call g_source_destroy(), so you'll need to do that explicitly before clearing it in cleanupSoupRequestOperation and sendRequestCallback.)
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:438
> > +    d->m_timeoutSource.clear();
> 
> Is this the right place to clear the timeout? We haven't received the response body yet at this point.

I checked the spec and it's not 100% clear on that. Although it indicates in the ProgressEvents table for ontimeout "When the author specified timeout has passed before the request could complete." So, let's indeed only cancel when the download completes.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:767
> > +            d->m_timeoutSource.set(soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle));
> 
> soup_add_timeout() and the related functions are broken in that they return a GSource that you don't own a reference on. As long as you aren't using multiple threads (which we aren't), then this won't cause problems, but when you switch to GRefPtr, make sure you *don't* use adoptGRef()

Okay, changed an using assignment only, put comments.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:868
> > +        if (d->m_firstRequest.timeoutInterval() > 0)
> > +            d->m_timeoutSource.set(soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, this));
> 
> should it cancel the timeout when defersLoading is true?

Makes sense, added.

So, Martin, Dan - what do you think, can we land it like this now?

[Dominik Röttsches (drott)]

Created attachment 163075 [details]
setTimeoutInterval support for soup, clear conditionally v6.

[Dominik Röttsches (drott)]

(In reply to comment #16)
> Created an attachment (id=163075) [details]
> setTimeoutInterval support for soup, clear conditionally v6.

Small update, g_source_destroy only if a source was created.

[Gustavo Noronha (kov)]

Comment on attachment 163075 [details]
setTimeoutInterval support for soup, clear conditionally v6.

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

Looks good to me, cq- because of the comments.

> Source/WebCore/ChangeLog:10
> +        has been successfully tested in combination with with my work-in-progress

with with

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:389
> +        g_source_destroy(d->m_timeoutSource.get());
> +        d->m_timeoutSource.clear();

So, what happens if a request times out? If I'm reading the patch right the callback is going to be called, which will call this function. Then the source will be destroyed (thus removed from the context and unrefed), then cleared here which will cause another unref, and then the callback will return FALSE, also causing it to be removed again. It looks like glib protects us from this case by checking whether the source has already been destroyed, though, so shouldn't be a problem... ok

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:768
> +            // soup_add_timeout returns a non-owned GSource*, not using adoptRef.

I think it would be better to be a bit more descriptive here, saying that soup_add_timeout returns a GSource whose only reference is owned by the context, so we need an additional one here.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:855
> +    // Except canceling a possible timeout timer, we only need to take action here to UN-defer loading.

canceling -> when canceling

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:874
> +        if (d->m_firstRequest.timeoutInterval() > 0)
> +            // soup_add_timeout returns a non-owned GSource*, not using adoptRef.
> +            d->m_timeoutSource = soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, this);

Please add braces to the if, so it's easier to read.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1001
> +    // Error domain is identical for ErrorsGtk and ErrorsEfl.

I think this comment isn't very useful, I'd take it out.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1007
> +    // Do not run this callback again.

Same here, this is pretty standard glib, so no need to be explicit.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1041
> +        if (d->m_firstRequest.timeoutInterval() > 0)
> +            // soup_add_timeout returns a non-owned GSource*, not using adoptRef.
> +            d->m_timeoutSource = soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle);

Ditto.

[Dominik Röttsches (drott)]

Created attachment 165781 [details]
Review comments addressed, v7.

[Dominik Röttsches (drott)]

(In reply to comment #18)
> (From update of attachment 163075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163075&action=review
> 
> Looks good to me, cq- because of the comments.

Thanks for the review!

> > Source/WebCore/ChangeLog:10
> > +        has been successfully tested in combination with with my work-in-progress
> 
> with with

Removed duplicate "with".

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:389
> > +        g_source_destroy(d->m_timeoutSource.get());
> > +        d->m_timeoutSource.clear();
> 
> So, what happens if a request times out? If I'm reading the patch right the callback is going to be called, which will call this function. Then the source will be destroyed (thus removed from the context and unrefed), then cleared here which will cause another unref, and then the callback will return FALSE, also causing it to be removed again. It looks like glib protects us from this case by checking whether the source has already been destroyed, though, so shouldn't be a problem... ok

Yes, I have the same understanding.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:768
> > +            // soup_add_timeout returns a non-owned GSource*, not using adoptRef.
> 
> I think it would be better to be a bit more descriptive here, saying that soup_add_timeout returns a GSource whose only reference is owned by the context, so we need an additional one here.

Comment changed to 
"// soup_add_timeout returns a GSource* whose only reference is owned by the context. We need to have our own reference to it, hence not using adoptRef."

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:855
> > +    // Except canceling a possible timeout timer, we only need to take action here to UN-defer loading.
> 
> canceling -> when canceling

Fixed.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:874
> > +        if (d->m_firstRequest.timeoutInterval() > 0)
> > +            // soup_add_timeout returns a non-owned GSource*, not using adoptRef.
> > +            d->m_timeoutSource = soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, this);
> 
> Please add braces to the if, so it's easier to read.

Done.

> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1001
> > +    // Error domain is identical for ErrorsGtk and ErrorsEfl.
> 
> I think this comment isn't very useful, I'd take it out.

Removed.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1007
> > +    // Do not run this callback again.
> 
> Same here, this is pretty standard glib, so no need to be explicit.

Removed.

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1041
> > +        if (d->m_firstRequest.timeoutInterval() > 0)
> > +            // soup_add_timeout returns a non-owned GSource*, not using adoptRef.
> > +            d->m_timeoutSource = soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle);
> 
> Ditto.

Done, and in one more place.

[WebKit Review Bot]

Comment on attachment 165781 [details]
Review comments addressed, v7.

Clearing flags on attachment: 165781

Committed r129636: <http://trac.webkit.org/changeset/129636>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.