| Summary: | [Soup] Cannot override default max-conns and max-conns-per-host Soup Session settings | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Marco Peereboom <marco> | ||||||||||||||||||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | landry, mrobinson, ryuan.choi, svillar, webkit.review.bot | ||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
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.
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(). Created attachment 100478 [details]
patch2
Created attachment 100492 [details]
patch3
Do what Landry suggested.
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. Created attachment 100542 [details]
patch4
pretty up changelog as requested.
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 > +2011-07-12 ./Source/WebCore/ChangeLog <marco@peereboom.us> > + 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. Created attachment 100554 [details]
patch5
Shuffle changelog to meet style requirements. Requested by Martin Robinson
This looks pretty good to me. I'd like Sergio to give it a look, just to double-check though. (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. (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? (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. (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. (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. 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.
Created attachment 100834 [details]
patch6
How about this instead?
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.
Comment on attachment 100834 [details] patch6 Attachment 100834 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9060296 Comment on attachment 100834 [details] patch6 Attachment 100834 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9065262 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. 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 ? > 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.
(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. 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.
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.
Created attachment 102030 [details]
patch8
One more time... One day I'll figure out the style.
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.
Y U COMPLAIN? I took this idiom from some other files. I don't get why it fails. Any clues? 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. 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. Created attachment 102146 [details]
patch9
incorporate martin's comments
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.
So this about what you meant? 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/ (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. I'll do a patch for that. Comment on attachment 102146 [details] patch9 Clearing flags on attachment: 102146 Committed r91954: <http://trac.webkit.org/changeset/91954> All reviewed patches have been landed. Closing bug. 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. (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. My bad; I merged the wrong diff. I swear one day I figure out these tools. Sorry about that. (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. |
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.