[libhijk-perl] 04/05: Fix what turned out to be a major regression reported in RT #101424
Robin Sheat
eythian-guest at moszumanska.debian.org
Thu Feb 12 21:55:47 UTC 2015
This is an automated email from the git hooks/post-receive script.
eythian-guest pushed a commit to annotated tag 0.19
in repository libhijk-perl.
commit 252a9d07946cf4533fafa12884a2a1d2ea474013
Author: Ævar Arnfjörð Bjarmason <avarab at gmail.com>
Date: Sat Jan 10 17:05:54 2015 +0000
Fix what turned out to be a major regression reported in RT #101424
We didn't spot this because we had no live tests that made enough
requests to kick in "Connection: close" on the remote end, now we have
that.
Note how early in the do {} block I've added a return of
$connection_close, I did this just for completeness, but I'm not aware
of any case where that would actually cause us to return an true
$connection_close.
Unifying some of this logic I added in 0.12-45-g972c2f2 also makes for
simpler code that doesn't branch as much depending on $head_as_array.
---
Changes | 13 +++++++++++++
lib/Hijk.pm | 57 +++++++++++++++++++++++++++++++++------------------------
t/live-unixis.t | 42 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 88 insertions(+), 24 deletions(-)
diff --git a/Changes b/Changes
index b075d96..cfaf6cf 100644
--- a/Changes
+++ b/Changes
@@ -1,9 +1,22 @@
0.19: #
+- Fix a major regression in 0.16. The introduction of "head_as_array"
+ completely broke the disconnection logic when the socket_cache was
+ enabled (which is the default). When talking to a webserver that
+ would disconnect us after N requests request N+1 would always fail
+ with a 0 byte response error.
+
+ This issue was reported as RT #101424
+ (https://rt.cpan.org/Public/Bug/Display.html?id=101424)
+
- Fix a minor regression in 0.16: The introduction of "head_as_array"
broke the "proto" part of the return value in a relatively obscure
edge case where we'd read the header & had no Content-Length and
couldn't read() anything.
+- Fix an edge case in the Trailer support. It would only kick in if we
+ got the Transfer-Encoding header before the "Trailer" header, not
+ the other way around.
+
0.18: # 2014-12-10T14:00:00+000
- We now do the right thing on "method => 'HEAD'". I.e. ignore the
Content-Length parameter, previously we'd just hang trying to slurp
diff --git a/lib/Hijk.pm b/lib/Hijk.pm
index c403cae..42a67ab 100644
--- a/lib/Hijk.pm
+++ b/lib/Hijk.pm
@@ -28,18 +28,19 @@ sub _read_http_message {
my $no_content_len = 0;
my $head = "";
my $method_is_head = do { no warnings qw(uninitialized); $method eq "HEAD" };
+ my $close_connection;
vec(my $rin = '', $fd, 1) = 1;
do {
- return (undef,undef,0,undef,undef, Hijk::Error::READ_TIMEOUT)
+ return ($close_connection,undef,0,undef,undef, Hijk::Error::READ_TIMEOUT)
if ((select($rin, undef, undef, $read_timeout) != 1) || (defined($read_timeout) && $read_timeout <= 0));
my $nbytes = POSIX::read($fd, $buf, $read_length);
- return (undef, $proto, $status_code, $header, $body)
+ return ($close_connection, $proto, $status_code, $header, $body)
if $no_content_len && $decapitated && (!defined($nbytes) || $nbytes == 0);
if (!defined($nbytes)) {
next if ($! == EWOULDBLOCK || $! == EAGAIN);
return (
- undef, undef, 0, undef, undef,
+ $close_connection, undef, 0, undef, undef,
Hijk::Error::RESPONSE_READ_ERROR,
"Failed to read http " . ($decapitated ? "body": "head") . " from socket",
$!+0,
@@ -49,7 +50,7 @@ sub _read_http_message {
if ($nbytes == 0) {
return (
- undef, undef, 0, undef, undef,
+ $close_connection, undef, 0, undef, undef,
Hijk::Error::RESPONSE_BAD_READ_VALUE,
"Wasn't expecting a 0 byte response for http " . ($decapitated ? "body": "head" ) . ". This shouldn't happen",
);
@@ -72,32 +73,42 @@ sub _read_http_message {
$status_code = substr($head, 9, 3);
substr($head, 0, index($head, $CRLF) + 2, ""); # 2 = length($CRLF)
- my ($doing_chunked, $content_length, $close_connection, $trailer_mode);
+ my ($doing_chunked, $content_length, $trailer_mode, $trailer_value_is_true);
for (split /${CRLF}/o, $head) {
my ($key, $value) = split /: /, $_, 2;
+ # Figure this out now so we don't need to scan the
+ # list later under $head_as_array, and just for
+ # simplicity and to avoid duplicating code later
+ # when !$head_as_array.
+ if ($key eq 'Transfer-Encoding' and $value eq 'chunked') {
+ $doing_chunked = 1;
+ } elsif ($key eq 'Content-Length') {
+ $content_length = $value;
+ } elsif ($key eq 'Connection' and $value eq 'close') {
+ $close_connection = 1;
+ } elsif ($key eq 'Trailer' and $value) {
+ $trailer_value_is_true = 1;
+ }
+
if ($head_as_array) {
push @$header => $key, $value;
-
- # Figure this out now so we don't need to scan
- # the list later.
- if ($key eq 'Transfer-Encoding' and $value eq 'chunked') {
- $doing_chunked = 1;
- } elsif ($key eq 'Content-Length') {
- $content_length = $value;
- } elsif ($key eq 'Connection' and $value eq 'close') {
- $close_connection = 1
- } elsif ($doing_chunked and $key eq 'Trailer' and $value) {
- $trailer_mode = 1;
- }
} else {
$header->{$key} = $value;
}
}
- if (($head_as_array and $doing_chunked)
- or
- (!$head_as_array and ($header->{'Transfer-Encoding'} and $header->{'Transfer-Encoding'} eq 'chunked'))) {
+ # We're processing the headers as a stream, and we
+ # only want to turn on $trailer_mode if
+ # Transfer-Encoding=chunked && Trailer=TRUE. However I
+ # don't think there's any guarantee that
+ # Transfer-Encoding comes before Trailer, so we're
+ # effectively doing a second-pass here.
+ if ($doing_chunked and $trailer_value_is_true) {
+ $trailer_mode = 1;
+ }
+
+ if ($doing_chunked) {
die "PANIC: The experimental Hijk support for chunked transfer encoding needs to be explicitly enabled with parse_chunked => 1"
unless $parse_chunked;
@@ -113,10 +124,8 @@ sub _read_http_message {
);
}
- if ($head_as_array and $content_length) {
+ if ($content_length) {
$read_length = $content_length - length($body);
- } elsif (!$head_as_array and $header->{'Content-Length'}) {
- $read_length = $header->{'Content-Length'} - length($body);
} else {
$read_length = 10204;
$no_content_len = 1;
@@ -124,7 +133,7 @@ sub _read_http_message {
}
}
} while( !$decapitated || (!$method_is_head && ($read_length > 0 || $no_content_len)) );
- return (undef, $proto, $status_code, $header, $body);
+ return ($close_connection, $proto, $status_code, $header, $body);
}
sub _read_chunked_body {
diff --git a/t/live-unixis.t b/t/live-unixis.t
new file mode 100644
index 0000000..2f5213d
--- /dev/null
+++ b/t/live-unixis.t
@@ -0,0 +1,42 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+use Hijk;
+use Test::More;
+use Test::Exception;
+
+unless ($ENV{TEST_LIVE}) {
+ plan skip_all => "Enable live testing by setting env: TEST_LIVE=1";
+}
+
+if($ENV{http_proxy}) {
+ plan skip_all => "http_proxy is set. We cannot test when proxy is required to visit u.nix.is";
+}
+
+for my $i (1..1000) {
+ lives_ok {
+ my $res = Hijk::request({
+ host => 'u.nix.is',
+ port => 80,
+ connect_timeout => 3,
+ read_timeout => 3,
+ path => "/?Hijk_test_nr=$i",
+ head => [
+ "X-Request-Nr" => $i,
+ "Referer" => "Hijk (file:" . __FILE__ . "; iteration: $i)",
+ ],
+
+ });
+
+ ok !exists($res->{error}), '$res->{error} does not exist, because we do not expect connect timeout to happen';
+ cmp_ok $res->{status}, '==', 200, "We got a 200 OK response";
+ if (exists $res->{head}->{Connection} and $res->{head}->{Connection} eq 'close') {
+ cmp_ok scalar(keys %{$Hijk::SOCKET_CACHE}), '==', 0, "We were told to close the connection. We should have no entry in the socket cache";
+ } else {
+ cmp_ok scalar(keys %{$Hijk::SOCKET_CACHE}), '==', 1, "We have an entry in the global socket cache";
+ }
+ } "We could make request number $i";
+}
+
+done_testing;
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-perl/packages/libhijk-perl.git
More information about the Pkg-perl-cvs-commits
mailing list