[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