After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 598948 - Add a function to obtain the SoupAddress of a certain host if a session has it cached
Add a function to obtain the SoupAddress of a certain host if a session has i...
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: API
unspecified
Other All
: Normal minor
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2009-10-19 15:39 UTC by José Millán Soto
Modified: 2009-11-23 14:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
soup_session_get_address_for_uri (1.74 KB, patch)
2009-10-19 15:41 UTC, José Millán Soto
needs-work Details | Review
soup-session-prepare-for-uri (12.47 KB, patch)
2009-10-22 15:25 UTC, José Millán Soto
none Details | Review
[SoupAddress] Remove some cruft (3.31 KB, patch)
2009-11-07 19:18 UTC, Dan Winship
committed Details | Review
[SoupAddress] make _resolve_async idempotent, document/fix thread-safety (10.11 KB, patch)
2009-11-07 19:18 UTC, Dan Winship
committed Details | Review
Add soup_session_prepare_for_uri (3.70 KB, patch)
2009-11-07 19:18 UTC, Dan Winship
committed Details | Review
New version of soup_session_prepare_for_uri, this times it checks whether there is a proxy or not (3.65 KB, patch)
2009-11-19 15:32 UTC, José Millán Soto
rejected Details | Review

Description José Millán Soto 2009-10-19 15:39:01 UTC
libsoup lacks the possibility of obtaining a SoupAddress of a host. This makes SoupSession API limited in some situations.
One situation where this function would be useful is prefetching DNS in WebKit (https://bugs.webkit.org/show_bug.cgi?id=23846 see comment 6)
Comment 1 José Millán Soto 2009-10-19 15:41:24 UTC
Created attachment 145792 [details] [review]
soup_session_get_address_for_uri

This patch includes the function soup_session_get_address_for_uri, which implements the functionality explained in bug description.
Comment 2 Dan Winship 2009-10-19 18:10:06 UTC
Comment on attachment 145792 [details] [review]
soup_session_get_address_for_uri

>+	SoupSessionHost *host = g_hash_table_lookup (priv->hosts, uri);

you need to hold priv->host_lock here, or else it's possible that priv->hosts will get modified by another thread while you're accessing it.

overall, the method seems a little bit... odd. It seems like it would be cleaner to have "soup_session_preresolve_address()" or something. Except that we'd need to have sync and async versions... I dunno.
Comment 3 Dan Winship 2009-10-20 15:35:02 UTC
OK, so, my problem with get_address_for_uri() is that really, it has no use *other* than DNS pre-resolving, so it's a little weird to have a "get address" API rather than the "prefetch DNS" API.

Also, if the user has an http proxy, then there's no point in resolving the host address ahead of time (since we're just going to pass it as a string to the proxy), but it *would* be useful to resolve the proxy address ahead of time (especially if they have PAC/WPAD and so the proxy might be different per-request-URI).

So what we should have is soup_session_prepare_for_uri() (alternate name suggestions welcome), which tells SoupSession that you're probably about to give it a request for that URI, and so it should... prepare for it. Initially this can just be hostname pre-resolving, but later we can add proxy pre-resolving too. (This would require reorganizing SoupSession some though.) It should call soup_address_resolve_async() using the session's async_context, and just document the fact that if you're using SoupSessionSync then either you need to set a useful async_context, or else you just shouldn't call this API. (It doesn't make any sense to have a blocking version of the call, since the whole point of the call is to avoid blocking later.)

One additional nit with the previous patch: it needs to actually create a SoupSessionHost for the hostname if one doesn't already exist, because otherwise it would only have an effect if you had already queued a message to that host, at which point you're probably already resolving the hostname anyway. So anyway, this probably means changing get_host_for_message() to just be a wrapper around a new get_host_for_uri(), and then using get_host_for_uri() from _prepare_for_uri() too.

Also, one more minor problem; if you call soup_address_resolve_async() twice on a given SoupAddress, then it will actually resolve the hostname twice. So that means if you start pre-resolving the address, and then actually queue the message for it before the address resolves, then it will actually send out a second DNS request and wait for *that one* to come back before it sends the message. (Though admittedly, the second request will probably come back faster than the first one did anyway, because the intermediate DNS servers should have cached the answer by that point.) With a little bit of rewriting, you could make SoupAddress notice when it was being called a second time when there was already one GResolver call pending, and it could just keep a list of callbacks to call when that completed.
Comment 4 José Millán Soto 2009-10-22 15:25:56 UTC
Created attachment 146046 [details] [review]
soup-session-prepare-for-uri

This patch solves most of the problems that were described in comment 3.

This patch defines soup_session_prepare_for_uri. Both SoupSessionAsync and SoupSessionSync have their own definition of prepare_for_uri; however, the one from SoupSessionSync do nothing.

soup_session_get_address_for_uri is now defined in soup-session-private.h, so only SoupSessionSync and SoupSessionAsync can use that (SoupSessionAsync uses it to obtain the address to prefetch). This function does now create a SoupSessionHost if it doesn't exist.

Also, SoupSessionAddress was modified so now if soup_session_address_resolve_async is called more than once while the resolution is being done it only does the resolution once and then all the callbacks are being called.

The only problem is that soup_session_prepare_for_uri does not yet handle proxies, but i think that it can be added later.
Comment 5 Dan Winship 2009-11-07 19:18:33 UTC
Created attachment 147183 [details] [review]
[SoupAddress] Remove some cruft
Comment 6 Dan Winship 2009-11-07 19:18:49 UTC
Created attachment 147184 [details] [review]
[SoupAddress] make _resolve_async idempotent, document/fix thread-safety

Document that _resolve_async() can be called multiple times, but only
from the same async_context, and _resolve_sync() can be called
multiple times from different threads. Note that _get_name, etc, may
misbehave in the presence of multiple threads.

Change _resolve_async() so that multiple attempts to resolve the same
address will result in only a single GResolver call, and they will all
complete at the same time.
Comment 7 Dan Winship 2009-11-07 19:18:55 UTC
Created attachment 147185 [details] [review]
Add soup_session_prepare_for_uri

Lets the session prepare for a possible upcoming request. (Eg,
WebKitGTK will call this when the user hovers over a link.) Currently
just does DNS resolution. Will eventually do proxy resolution as well.

Based on a patch from José Millán Soto
Comment 8 Dan Winship 2009-11-07 19:19:56 UTC
does this work? i think it's ready to go
Comment 9 José Millán Soto 2009-11-19 15:32:28 UTC
Created attachment 148129 [details] [review]
New version of soup_session_prepare_for_uri, this times it checks whether there is a proxy or not

Patch based on the previous one sent by Dan.
This one is basically the same idea of the one Dan sent, but if checks is there is a proxy configured, and if so, it does nothing (to prevent prefetching a host which may not be used)

I think that this is ok enought for going as a first version of the function.
Comment 10 Dan Winship 2009-11-22 01:05:45 UTC
Attachment 147185 [details] pushed as f8ce30a - Add soup_session_prepare_for_uri

Just checking whether or not a proxyresolver is present isn't enough,
since that doesn't necessarily mean a given host will use a proxy. So I
committed my original patch for now.

(Note that this is committed to master, not the gnome-2-28 branch.)
Comment 11 José Millán Soto 2009-11-23 10:22:39 UTC
> Just checking whether or not a proxyresolver is present isn't enough,
> since that doesn't necessarily mean a given host will use a proxy. So I
> committed my original patch for now.

My idea was not to check whether a host will use a proxy, it was to ensure not to resolv a DNS address if there is the posibility of the address using proxy.
Comment 12 Dan Winship 2009-11-23 14:49:36 UTC
Right, but in epiphany, there will *always* be a SoupProxyResolver attached to the session, whether or not the user has a proxy configured. (Because adding a proxyresolver to the session is the only way to even find out if a proxy is configured.) So your patch would have made epiphany never pre-resolve hostnames. Whereas without that patch, we do some gratuitous extra DNS resolution sometimes, but that's not too big a deal.