This queue is for tickets about the POE-Component-Client-HTTP CPAN distribution.

Report information
The Basics
Id:
104056
Status:
new
Priority:
Low/Low

People
Owner:
Nobody in particular
Requestors:
perl [...] pied.nu
Cc:
AdminCc:

BugTracker
Severity:
(no value)
Broken in:
(no value)
Fixed in:
(no value)

Attachments
Philip_Gwyn-PoCo-Client-HTTP.reuse-retry.01.patch



Subject: Better handling of IO errors on reused sockets
Included is a patch and test that will retry a connection if we get an IO error on a reused socket. https://developer.mozilla.org/en-US/docs/Web/HTTP/Pipelining_FAQ seems to imply this is the correction behaviour. Note that currently the patch will retry if we received a partial HTTP/1.1 header before the error. I'm no longer convinced this is correct behaviour. Also - we might want an option to turn this behaviour off.
Subject: Philip_Gwyn-PoCo-Client-HTTP.reuse-retry.01.patch
diff -rub POE-Component-Client-HTTP-0.949/MANIFEST POE-Component-Client-HTTP-0.949-PG1/MANIFEST --- POE-Component-Client-HTTP-0.949/MANIFEST 2014-07-08 14:08:28.000000000 -0400 +++ POE-Component-Client-HTTP-0.949-PG1/MANIFEST 2015-04-28 15:07:35.188755041 -0400 @@ -45,3 +45,4 @@ t/60_rt50231_pending_many.t t/release-pod-coverage.t t/release-pod-syntax.t +t/61_keepalive_problem.t Only in POE-Component-Client-HTTP-0.949-PG1: MYMETA.json Only in POE-Component-Client-HTTP-0.949-PG1: MYMETA.yml Only in POE-Component-Client-HTTP-0.949-PG1: Makefile diff -rub POE-Component-Client-HTTP-0.949/Makefile.PL POE-Component-Client-HTTP-0.949-PG1/Makefile.PL --- POE-Component-Client-HTTP-0.949/Makefile.PL 2014-07-08 14:08:28.000000000 -0400 +++ POE-Component-Client-HTTP-0.949-PG1/Makefile.PL 2015-04-28 15:38:58.035304346 -0400 @@ -33,7 +33,7 @@ "Test::POE::Server::TCP" => "1.14", "URI" => "1.37" }, - "VERSION" => "0.949", + "VERSION" => "0.94901", "test" => { "TESTS" => "t/*.t" } Only in POE-Component-Client-HTTP-0.949-PG1: Makefile.PL~ Only in POE-Component-Client-HTTP-0.949-PG1: blib diff -rub POE-Component-Client-HTTP-0.949/lib/POE/Component/Client/HTTP/Request.pm POE-Component-Client-HTTP-0.949-PG1/lib/POE/Component/Client/HTTP/Request.pm --- POE-Component-Client-HTTP-0.949/lib/POE/Component/Client/HTTP/Request.pm 2014-07-08 14:08:28.000000000 -0400 +++ POE-Component-Client-HTTP-0.949-PG1/lib/POE/Component/Client/HTTP/Request.pm 2015-04-28 16:28:13.153812860 -0400 @@ -48,6 +48,7 @@ REQ_FACTORY => 16, REQ_CONN_ID => 17, REQ_PEERNAME => 18, + REQ_CACHE => 19, }; use constant { @@ -73,7 +74,7 @@ REQ_ID REQ_POSTBACK REQ_CONNECTION REQ_HTTP_REQUEST REQ_STATE REQ_RESPONSE REQ_BUFFER REQ_OCTETS_GOT REQ_TIMER REQ_PROG_POSTBACK REQ_USING_PROXY REQ_HOST REQ_PORT - REQ_HISTORY REQ_START_TIME REQ_CONN_ID REQ_PEERNAME + REQ_HISTORY REQ_START_TIME REQ_CONN_ID REQ_PEERNAME REQ_CACHE ) ) { no strict 'refs'; @@ -170,6 +171,7 @@ $factory, # REQ_FACTORY undef, # REQ_CONN_ID undef, # REQ_PEERNAME + undef, # REQ_CACHE ]; return bless $self, $class; } @@ -232,12 +234,7 @@ # ignore content for HEAD requests. This may thwart keep-alive, # however. - if ( - $self->[REQ_HTTP_REQUEST]->method() ne "HEAD" and - defined $self->[REQ_RESPONSE]->content_length and - not defined $self->[REQ_RESPONSE]->header("Transfer-Encoding") and - $self->[REQ_OCTETS_GOT] < $self->[REQ_RESPONSE]->content_length - ) { + if ( $self->__is_incomplete ) { DEBUG and warn( "got " . $self->[REQ_OCTETS_GOT] . " of " . $self->[REQ_RESPONSE]->content_length @@ -256,6 +253,14 @@ } } +sub __is_incomplete { + my( $self ) = @_; + return 1 if $self->[REQ_HTTP_REQUEST]->method() ne "HEAD" and + defined $self->[REQ_RESPONSE]->content_length and + not defined $self->[REQ_RESPONSE]->header("Transfer-Encoding") and + $self->[REQ_OCTETS_GOT] < $self->[REQ_RESPONSE]->content_length; + return; +} sub add_content { my ($self, $data) = @_; Only in POE-Component-Client-HTTP-0.949-PG1/lib/POE/Component/Client/HTTP: Request.pm~ diff -rub POE-Component-Client-HTTP-0.949/lib/POE/Component/Client/HTTP.pm POE-Component-Client-HTTP-0.949-PG1/lib/POE/Component/Client/HTTP.pm --- POE-Component-Client-HTTP-0.949/lib/POE/Component/Client/HTTP.pm 2014-07-08 14:08:28.000000000 -0400 +++ POE-Component-Client-HTTP-0.949-PG1/lib/POE/Component/Client/HTTP.pm 2015-04-28 16:43:53.426527817 -0400 @@ -269,6 +269,15 @@ $http_request, $response_event, $tag, $progress_event, $proxy_override, $sender ); + + _setup_request( $heap, $request ); +} + +sub _setup_request { + my( $heap, $request ) = @_; + + my $http_request = $request->[REQ_HTTP_REQUEST]; + $heap->{request}->{$request->ID} = $request; $heap->{ext_request_to_int_id}->{$http_request} = $request->ID; @@ -303,12 +312,23 @@ } } +sub _retry_request { + my( $heap, $request ) = @_; + DEBUG and + warn "Problem with a cached connection; retrying"; + $request->[REQ_CACHE] = undef(); + _setup_request( $heap, $request ); +} + sub _poco_weeble_connect_done { my ($heap, $response) = @_[HEAP, ARG0]; my $connection = $response->{'connection'}; my $request_id = $response->{'context'}; + my $from_cache = $response->{'from_cache'}||''; + DEBUG and + warn "connect_done connection=$connection from_cache=$from_cache"; # Can't handle connections if we're shut down. # TODO - How do we still get these? Were they previously queued or @@ -347,6 +367,7 @@ $heap->{wheel_to_request}->{ $new_wheel->ID() } = $request_id; $request->[REQ_CONNECTION] = $connection; + $request->[REQ_CACHE] = $from_cache; # SSLify needs us to call it's function to get the "real" socket my $peer_addr; @@ -552,6 +573,12 @@ $request->[REQ_STATE] & (RS_IN_CONTENT | RS_DONE) and not $request->[REQ_STATE] & RS_POSTED ) { + if( $request->[REQ_CACHE] && $request->__is_incomplete ) { + # if we get an error on a cached socket, we naively try again + _retry_request( $heap, $request ); + return; + } + _finish_request($heap, $request); return; } @@ -570,29 +597,27 @@ my $text = join '' => @$lines; DEBUG and warn "Got ", length($text), " bytes of data without LF."; - # If we have data, build and return a response from it. - if ($text =~ /\S/) { - DEBUG and warn( - "Generating HTTP response for HTTP/0.9 response without LF." - ); - $request->[REQ_RESPONSE] = HTTP::Response->new( - 200, 'OK', [ - 'Content-Type' => 'text/html', - 'X-PCCH-Peer' => $request->[REQ_PEERNAME], - ], $text - ); - $request->[REQ_RESPONSE]->protocol('HTTP/0.9'); - $request->[REQ_RESPONSE]->request($request->[REQ_HTTP_REQUEST]); - $request->[REQ_STATE] = RS_DONE; - $request->return_response; + if( _is_ancient_response( $text ) ) { + _ancient_response( $request, $text ); return; } + if( $request->[REQ_CACHE] ) { + # if we get an error on a cached socket, we naively try again + _retry_request( $heap, $request ); + return; + } # No data received. This is an incomplete response. $request->error(400, "Incomplete response - $request_id"); return; } + if( $request->[REQ_CACHE] ) { + # if we get an error on a cached socket, we naively try again + _retry_request( $heap, $request ); + return; + } + # We haven't built a proper response, and nothing returned by the # server can be turned into a proper response. Send back an error. # Changed to 406 after considering rt.cpan.org 20975. @@ -607,6 +632,33 @@ $request->error(406, "Server response is Not Acceptable - $request_id"); } +# Is this block of text a partial header? +# Used by io_error to see # if we should retry the request +sub _is_ancient_response { + my( $text ) = @_; + return if $text =~ m(^HTTP/1); # start of status line + return 1 if $text =~ /\S/; + return; +} + +# Build a HTTP/0.9 response +sub _ancient_response { + my( $request, $text ) = @_; + # If we have data, build and return a response from it. + DEBUG and warn( + "Generating HTTP response for HTTP/0.9 response without LF." + ); + $request->[REQ_RESPONSE] = HTTP::Response->new( + 200, 'OK', [ + 'Content-Type' => 'text/html', + 'X-PCCH-Peer' => $request->[REQ_PEERNAME], + ], $text + ); + $request->[REQ_RESPONSE]->protocol('HTTP/0.9'); + $request->[REQ_RESPONSE]->request($request->[REQ_HTTP_REQUEST]); + $request->[REQ_STATE] = RS_DONE; + $request->return_response; +} #------------------------------------------------------------------------------ # Read a chunk of response. This code is directly adapted from Artur @@ -1166,6 +1218,12 @@ including how to alter the connection manager's resolver configuration (for example, to force IPv6 or prefer it before IPv4). +If a request on a connection that was reused by the ConnectionManager +encounters IO errors, then POE::Component::Client::HTTP will naively retry +the request on a new connection. This does not apply to fresh connections +created by the ConnectionManager. This behaviour is based on the discussion +at L<https://developer.mozilla.org/en-US/docs/Web/HTTP/Pipelining_FAQ>. + =item CookieJar => $cookie_jar C<CookieJar> sets the component's cookie jar. It expects the cookie Only in POE-Component-Client-HTTP-0.949-PG1/lib/POE/Component/Client: HTTP.pm~ Only in POE-Component-Client-HTTP-0.949-PG1: pm_to_blib Only in POE-Component-Client-HTTP-0.949-PG1/t: 53_response_parser.t~ diff -rub POE-Component-Client-HTTP-0.949/t/61_keepalive_problem.t POE-Component-Client-HTTP-0.949-PG1/t/61_keepalive_problem.t --- POE-Component-Client-HTTP-0.949/t/61_keepalive_problem.t 2015-04-28 15:41:42.706441193 -0400 +++ POE-Component-Client-HTTP-0.949-PG1/t/61_keepalive_problem.t 2015-04-28 16:42:04.712507257 -0400 @@ -0,0 +1,161 @@ +#!/usr/bin/perl +# vim: ts=2 sw=2 filetype=perl expandtab + +# Philpi Gwyn's test for IO errors on a Keepalive connection + +use warnings; +use strict; + +BEGIN { + my @proxies = grep /^http.*proxy$/i, keys %ENV; + delete @ENV{@proxies} if @proxies; +} + +use Test::More; +use Test::POE::Server::TCP; +use POE qw(Filter::Stream Component::Client::HTTP); +use HTTP::Request::Common qw(GET); + +POE::Component::Client::HTTP->spawn( Alias => 'ua' ); + +plan tests => 18; + +POE::Session->create( + inline_states => { + _start => \&start, + testd_registered => \&testd_start, + testd_client_input => \&testd_input, + first_response => \&first_response, + second_response => \&second_response, + third_response => \&third_response, + done => \&done, + }, +); + +sub start { + my $heap = $_[HEAP]; + + $heap->{testd} = Test::POE::Server::TCP->spawn( + Filter => POE::Filter::Stream->new, + address => 'localhost', + ); +} + +sub testd_start { + my ($kernel, $heap) = @_[KERNEL, HEAP]; + + my $port = $heap->{testd}->port; + $heap->{port} = $port; + + # Fetch a URL that has no content. + $kernel->post( + 'ua', 'request', 'first_response', + GET "http://localhost:$port/misc/first.txt" + ); +}; + +sub done { + $_[HEAP]->{testd}->shutdown; + $_[KERNEL]->post( ua => 'shutdown' ); +} + +####################################################################### +sub first_response { + my( $kernel, $heap, $req_p, $resp_p ) = @_[KERNEL, HEAP, ARG0..$#_]; + my $req = $req_p->[0]; + my $resp = $resp_p->[0]; + + ok( $resp->is_success, "Successful request" ) or die $resp->as_string; + like( $resp->content, qr(first response), "First" ); + + $heap->{once} = 1; + $kernel->post( + 'ua', 'request', 'second_response', + GET "http://localhost:$heap->{port}/misc/second.txt" + ); +} + +sub second_response { + my( $kernel, $heap, $req_p, $resp_p ) = @_[KERNEL, HEAP, ARG0..$#_]; + my $req = $req_p->[0]; + my $resp = $resp_p->[0]; + + ok( $resp->is_success, "Successful request" ) or die $resp->as_string; + like( $resp->content, qr(second response), "Second" ) or die $resp->as_string; + $heap->{once} = 1; + $kernel->post( + 'ua', 'request', 'third_response', + GET "http://localhost:$heap->{port}/misc/third.txt" + ); +} + +sub third_response { + my( $kernel, $heap, $req_p, $resp_p ) = @_[KERNEL, HEAP, ARG0..$#_]; + my $req = $req_p->[0]; + my $resp = $resp_p->[0]; + + ok( $resp->is_success, "Successful request" ) or die $resp->as_string; + like( $resp->content, qr(third response), "Third" ) or die $resp->as_string; + + $kernel->yield( 'done' ); +} + +####################################################################### +sub testd_input { + my ($kernel, $heap, $id, $input) = @_[KERNEL, HEAP, ARG0, ARG1]; + + my $resp = HTTP::Response->new( 200, 'OK' ); + $resp->content_type( 'text/plain' ); + my $I = $heap->{testd}->client_info( $id ); + my $info = join '->', "$I->{peeraddr}:$I->{peerport}", + "$I->{sockaddr}:$I->{sockport}"; + pass( "Input $info" ); + + if( $input =~ m(^GET /misc/first) ) { + $heap->{seen} = $info; + $resp->content( 'first response' ); + } + elsif( $input =~ m(^GET /misc/second) ) { + if( $heap->{once} > 0 ) { + is( $heap->{seen}, $info, "Reused connection" ); + pass( "Creating an error" ); + $heap->{once} = 0; + $heap->{testd}->terminate( $id ); # close the connection + return; + } + isnt( $heap->{seen}, $info, "New connection" ); + $resp->content( 'second response' ); + $heap->{seen} = $info; + } + elsif( $input =~ m(^GET /misc/third) ) { + # XXX - this doesn't work, because Client::HTTP is interpreting + # a partial HTTP/1.1 header as a HTTP/0.9 header. + if( $heap->{once} > 0 ) { + is( $heap->{seen}, $info, "Reused connection" ); + pass( "Creating an error" ); + $heap->{once} = 0; + $heap->{testd}->disconnect( $id ); + $resp->content_length( 300 ); + my $data = "HTTP/1.1 ".$resp->as_string; + $heap->{testd}->send_to_client( $id, $data ); + return; + } + isnt( $heap->{seen}, $info, "New connection" ); + $resp->content( 'third response' ); + } + else { + $resp->code( 400 ); + $resp->message( "Bad request" ); + $resp->content( 'Bad request\n$input' ); + } + + $resp->content_length( length $resp->content ); + my $data = "HTTP/1.1 ".$resp->as_string; + # warn "data='$data'"; + $heap->{testd}->send_to_client($id, $data); +} + +####################################################################### +POE::Kernel->run(); +pass( "Sane shutdown" ); +exit; Only in POE-Component-Client-HTTP-0.949-PG1/t: 61_keepalive_problem.t~


This service runs on Request Tracker, is sponsored by The Perl Foundation, and maintained by Best Practical Solutions.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.