Bug 64355 – [Soup] Cannot override default max-conns and max-conns-per-host Soup Session settings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Marco Peereboom]

Created attachment 100476 [details]
patch

Both max-conns and max-conns-per-host are not honored.  This makes the
following code not do what one would expect:

        session = webkit_get_default_session();
        g_object_set(session, "max-conns", max_connections, (char *)NULL);
        g_object_set(session, "max-conns-per-host", max_host_connections,
            (char *)NULL);

Problem is that webkit will overwrite these values upon "first contact"
with a site.  The responsible file is:
WebCore/platform/network/soup/ResourceHandleSoup.cpp in function
static void ensureSessionIsInitialized(SoupSession* session)
    static const int maxConnections = 60;
    static const int maxConnectionsPerHost = 6;
...
    g_object_set(session,
                 SOUP_SESSION_MAX_CONNS, maxConnections,
                 SOUP_SESSION_MAX_CONNS_PER_HOST, maxConnectionsPerHost,
                 NULL);

This code is run AFTER I set those values.

Attached is a patch that fixes this.

[WebKit Review Bot]

Attachment 100476 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Landry Breuil]

I can't comment if this effectively fixes the issue as i can't test it now, but if ensureSessionIsInitialized() is called is ResourceHandle::defaultSession(), then the other ensureSessionIsInitialized() calls in startHTTPRequest() and startNonHTTPRequest() (and maybe others) should be removed, since they are just after returning from defaultSession().

[Marco Peereboom]

Created attachment 100478 [details]
patch2

[Marco Peereboom]

Created attachment 100492 [details]
patch3

Do what Landry suggested.

[Martin Robinson]

Comment on attachment 100492 [details]
patch3

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

Seems like a decent change, but please touch up the ChangeLog a bit.

> Source/WebCore/ChangeLog:6
> +        max-conns and max-conns-per-host not honored
> +        https://bugs.webkit.org/show_bug.cgi?id=64355
> +
> +        Reviewed by NOBODY (OOPS!).

I think this change deserves a line or two explaining what problem this fixes.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

You should either add a test or explain here why you didn't.

> Source/WebCore/ChangeLog:12
> +        * platform/network/soup/ResourceHandleSoup.cpp:
> +        (WebCore::ResourceHandle::defaultSession):
> +

Typically we fill these out.

[Marco Peereboom]

Created attachment 100542 [details]
patch4

pretty up changelog as requested.

[Martin Robinson]

Comment on attachment 100542 [details]
patch4

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

Sorry for the confusion. Comments below.

> Source/WebCore/ChangeLog:2
> [phone]  ./Source/WebCore/ChangeLog  <[e-mail]>
> +

This line should list your name. I'm not sure what went wrong here.

> Source/WebCore/ChangeLog:7
> +        Default max-conns and max-conns-per-host are set at "first contact" with
> +        a site instead of at creation time.  This results in values being
> +        overwritten if they are set prior to said "first contact"; which is the
> +        most likely (or only) scenario.
> +        https://bugs.webkit.org/show_bug.cgi?id=64355

The format should actually be:

<bug title>
<bug URL>

<reviewer -- OOPS is fine here, the tools will take care of it>

<change description>

> Source/WebCore/ChangeLog:12
> +        Rigged libsoup and xxxterm web browser to diagnose the issue and
> +        validate the patch.

Here you should preserve "No new tests." and then followed by a description of why tests were not added to the WebKit repository itself.

[Marco Peereboom]

Created attachment 100554 [details]
patch5

Shuffle changelog to meet style requirements.  Requested by Martin Robinson

[Martin Robinson]

This looks pretty good to me. I'd like Sergio to give it a look, just to double-check though.

[Sergio Villar Senin]

(In reply to comment #9)
> This looks pretty good to me. I'd like Sergio to give it a look, just to double-check though.

The change looks sane but we would be breaking the contract of webkit_get_default_session(). With the current code you could do things like:

session = webkit_get_default_session();
soup_session_add_feature(session, my_cookie_jar);

If you do this before any request, then webkitgtk+ will use my_cookie_jar instead of the default.

But, with this patch, after webkit_get_default_session() you will get a SoupSession with a default CookieJar and a default SoupRequester among other things. It's true that you still could remove them and add yours but I think this could be considered an API break.

[Martin Robinson]

(In reply to comment #10)

> But, with this patch, after webkit_get_default_session() you will get a SoupSession with a default CookieJar and a default SoupRequester among other things. It's true that you still could remove them and add yours but I think this could be considered an API break.

Will the current code also override any CookieJar and SoupRequester that the user sets, in the same way that it's overriding these other settings?

[Sergio Villar Senin]

(In reply to comment #11)
> (In reply to comment #10)
> 
> > But, with this patch, after webkit_get_default_session() you will get a SoupSession with a default CookieJar and a default SoupRequester among other things. It's true that you still could remove them and add yours but I think this could be considered an API break.
> 
> Will the current code also override any CookieJar and SoupRequester that the user sets, in the same way that it's overriding these other settings?

It will not because we first check if there is any cookiejar or souprequester before setting the default ones.

[Martin Robinson]

(In reply to comment #12)

> It will not because we first check if there is any cookiejar or souprequester before setting the default ones.

Ah, so this would just make the behavior slightly more consistent. Another approach would be to check if the max-conns and max-conns-per-host differ from the default. That seems like a bit of a hack though.

[Sergio Villar Senin]

(In reply to comment #13)
> (In reply to comment #12)
> 
> > It will not because we first check if there is any cookiejar or souprequester before setting the default ones.
> 
> Ah, so this would just make the behavior slightly more consistent. Another approach would be to check if the max-conns and max-conns-per-host differ from the default. That seems like a bit of a hack though.

Another possibility could be to remove that from webkitgtk+, as it is a very specific browser feature, (actually we were setting as defaults the values used by most of the browsers) and let browsers decide.

[Marco Peereboom]

Actually moving
    g_object_set(session,
                 SOUP_SESSION_MAX_CONNS, maxConnections,
                 SOUP_SESSION_MAX_CONNS_PER_HOST, maxConnectionsPerHost,
                 NULL);
into SoupSession* ResourceHandle::defaultSession() might be a better idea while leaving the rest intact.  I agree all options are a bit meh but currently it is a bug that on low FD systems results in crashes.

[Marco Peereboom]

Created attachment 100834 [details]
patch6

How about this instead?

[WebKit Review Bot]

Attachment 100834 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/network/soup/Resou..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:847:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:857:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 100834 [details]
patch6

Attachment 100834 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9060296

[Gustavo Noronha (kov)]

Comment on attachment 100834 [details]
patch6

Attachment 100834 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9065262

[Marco Peereboom]

meh angry robots are angry but please look at the diff and let me know if the idea is right and then I'll move the code around to make them angry robots happy again.

[Landry Breuil]

Can we get a followup on that ? What's the best approach to take, so that we can fix the issue locally once a proper patch is accepted ? I'd be fine if we did what's in comment 13,ie check if value differ from default before setting it, but then that'd mean there's already a sane default in soup itself ?
Or we should just remove those two g_object_set and use the real defaults provided by soup as suggested in #14 ?

[Marco Peereboom]

> Or we should just remove those two g_object_set and use the real defaults provided by soup as suggested in #14 ?

I really wish we would not remove the ability to set these values.  Webkit runs in embedded environments where resources are scarce.  The fact that soup doesn't handle closed connections properly is enough of an issue.  Removing these knobs would exacerbate the situation.

I'll roll a new patch later today to address this a bit better because the last one I proposed is crap.

[Landry Breuil]

(In reply to comment #22)
> > Or we should just remove those two g_object_set and use the real defaults provided by soup as suggested in #14 ?
> 
> I really wish we would not remove the ability to set these values.  Webkit runs in embedded environments where resources are scarce.  The fact that soup doesn't handle closed connections properly is enough of an issue.  Removing these knobs would exacerbate the situation.

I was talking of removing the two g_object_set from webkit itself in ResourceHandleSoup.cpp, letting the possibility to the user of the API to do them.

[Marco Peereboom]

Created attachment 102027 [details]
patch7

Patch moves g_object_set for session parameters into defaultSession.  Also make sure the values are only set the first time the function is invoked.

[WebKit Review Bot]

Attachment 102027 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:852:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marco Peereboom]

Created attachment 102030 [details]
patch8

One more time... One day I'll figure out the style.

[WebKit Review Bot]

Attachment 102030 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:854:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marco Peereboom]

Y U COMPLAIN?

I took this idiom from some other files.  I don't get why it fails.  Any clues?

[Martin Robinson]

Comment on attachment 102030 [details]
patch8

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

This looks like a good solution to me. Thanks for working on it. If you can get some buy-in from Sergio and some of the others who've commented here. I'll r+ this with the small fixes I suggested below.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:849
> +    initData = g_object_get_data(G_OBJECT(session), "webkit-init");

I'd prefer smething like this:

static SoupSession* session = 0;
if (!session) {
    session = soup_session_async_new();
    g_object_set(...);
}

Depending on "webkit-init" adds a dependency on some other random bit of the code.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:850
> +    if (!initData)

You should use curly braces on this if block, since the contents span more than one line.

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:854
>> +                         NULL);
> 
> Use 0 instead of NULL.  [readability/null] [5]

You can ignore this failure. The style-bot isn't smart enough to understand that the NULL is necessary because the g_object_set is on another line.

[Marco Peereboom]

Ok this is a bit closer to the previous patch but looking at other code in webkit I figured the static init stuff uses this idiom.  I'll roll a new patch in the morning.

[Marco Peereboom]

Created attachment 102146 [details]
patch9

incorporate martin's comments

[WebKit Review Bot]

Attachment 102146 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:853:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marco Peereboom]

So this about what you meant?

[Sergio Villar Senin]

Patch looks good to me.

Martin what about using this patch to change the maxConnections value? The most typical  value used by the other browsers is around 30-35, and we are using 60 instead. Also I'd change the url in the comment to point to http://www.browserscope.org/

[Martin Robinson]

(In reply to comment #34)
> Patch looks good to me.
> 
> Martin what about using this patch to change the maxConnections value? The most typical  value used by the other browsers is around 30-35, and we are using 60 instead. Also I'd change the url in the comment to point to http://www.browserscope.org/

That sounds like a great change, but I think it should happen in another patch/bug.

[Marco Peereboom]

I'll do a patch for that.

[WebKit Review Bot]

Comment on attachment 102146 [details]
patch9

Clearing flags on attachment: 102146

Committed r91954: <http://trac.webkit.org/changeset/91954>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Landry Breuil]

Fwiw, the Changelog entry is wrong in the commit.. it talks about platform/graphics/FontFallbackList.cpp whereas Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp was modified.

[Martin Robinson]

(In reply to comment #39)
> Fwiw, the Changelog entry is wrong in the commit.. it talks about platform/graphics/FontFallbackList.cpp whereas Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp was modified.

Thanks for the notice! I've corrected this in http://trac.webkit.org/changeset/91976. Marco, please use prepare-ChangeLog to generate your ChangeLogs.

[Marco Peereboom]

My bad; I merged the wrong diff.  I swear one day I figure out these tools.  Sorry about that.

[Martin Robinson]

(In reply to comment #41)
> My bad; I merged the wrong diff.  I swear one day I figure out these tools.  Sorry about that.

No problem! Thanks for the patch.