Hi Gisle,
On 10/04/11 00:06, Gisle Aas wrote:
I'm thinking that we should instead document the {proxy} attribute. It can be
useful to have it available in $response->request to be able to tell where the
request was sent.
The reason why _need_proxy() does not override {proxy} if set is that it allows
other handlers to set it up. You might for instance want to set up a handler
that proxies everything not going to your intranett, or just set it explicitly
for some requests.
I agree that documenting the {proxy} attribute is a good idea.
If the behaviour of _need_proxy() is going to stay the same perhaps the
documentation for LWP::UserAgent::proxy() could be updated to include a
warning, something like:
Note that if a HTTP::Request object already has a proxy attribute
set, the LWP::UserAgent proxy setting will not be used.
If the proxy attribute is going to be documented in HTTP::Request it
would be nice if there was an accessor method added so that the proxy
could be set on a request in the same way that is it added to a user agent.
One more thought - though I think this might complicate the
LWP::UserAgent interface in a non-userfriendly way - what do you think
of adding a switch to LWP::UserAgent so that the proxy setting could be
set to override the request proxy setting (but not by default)?
I'm happy to write the patches for these changes (once we've established
what they should be).
Cheerio,
Brad
--Gisle
On Apr 9, 2011, at 0:44 , Bradley Dean wrote:
Greetings,
I've been looking at what feels to me like a bug with the handling of proxy
configuration. When a HTTP::Request is made through an LWP::UserAgent with a
proxy configured the HTTP::Request is changed by LWP::UserAgent::__need_proxy:
sub _need_proxy {
...
$req->{proxy} = $HTTP::URI_CLASS->new($proxy);
}
"proxy" is not a documented attribute of the HTTP::Request class, but when it
is set _need_proxy ignores the current LWP::UserAgent proxy settings:
sub _need_proxy {
my($req, $ua) = @_;
return if exists $req->{proxy};
...
}
This becomes a problem when trying to use a single HTTP::Request instance
across multiple proxies - for example:
my $request = HTTP::Request->new(GET => 'http://www.example.com/');
my $ua = LWP::UserAgent->new();
$ua->proxy('http', 'http://proxy_one.example.org:12345/');
# This first request goes through proxy_one
$ua->request($request);
# Now change the proxy on the LWP::UserAgent
$ua->proxy('http', 'http://proxy_two.example.org:12345/');
# This second request goes through proxy_one still because
# $request->{proxy} == "http://proxy_one.example.org:12345/";
$ua->request($request);
This can be worked around in the calling software by deleting $request->{proxy}
between requests but it's a hack (and one which could fail at any time given that
this is a non-documented attribute).
I've thought of a few possible patches:
PATCH 1:
Remove the "return if exists $req->{proxy};" line from _need_proxy. I think
this is the cleanest approach.
Thinking of reasons not to do this: that it incurs some extra processing cost per request
(but only in the case that a single request is used over and over again); it might break
existing code that relies on the undocumented "proxy" attribute (where people
are building requests with proxies rather than configuring via LWP::UserAgent). If this
is a concern see 2.
PATCH 1 GIT:
diff --git a/lib/LWP/UserAgent.pm b/lib/LWP/UserAgent.pm
index 1ee9ffb..945cd8a 100644
--- a/lib/LWP/UserAgent.pm
+++ b/lib/LWP/UserAgent.pm
@@ -934,7 +934,6 @@ sub mirror
sub _need_proxy {
my($req, $ua) = @_;
- return if exists $req->{proxy};
my $proxy = $ua->{proxy}{$req->uri->scheme} || return;
if ($ua->{no_proxy}) {
if (my $host = eval { $req->uri->host }) {
PATCH 2:
A more expensive option would be to check if the LWP::UserAgent has a proxy
configured and if it is different from the proxy attribute on the HTTP::Request.
If it is different I would argue that LWP::UserAgent value should override the
requestm, so if the values were different the LWP::UserAgent could change the
value of the proxy atttribute.
I've not included a git patch for this option but could supply one if it was of
interest.
PATCH 3:
At the moment $request->{proxy} has to be set because the request is passed out
of the LWP::UserAgent to a protocol handler. As I recall at this stage the
LWP::UserAgent is not visible to the protocol handler, so $request is used to pass
on the proxy configuration.
A much bigger change to the code would have the protocol handler given access
to the LWP::UserAgent so that the proxy did not have to be attached to the
HTTP::Request. The same decision still needs to be made as in PATCH 2 (ie what
happens if someone has set the proxy attribute).
As with patch 2, I've not included a git patch for this option but could supply
one if it was of interest.
Cheerio,
Brad
--
Bradley Dean
Email: bjd...@bjdean.id.au Skype: sk...@bjdean.id.au
Mobile(Aus): +61-413014395 WWW: http://bjdean.id.au/
S/MIME: http://bjdean.id.au/certs/email-public-rsa.txt
