On Thu, Mar 27, 2014 at 09:20:20PM +0100, Uwe Storbeck wrote: > Hi Mark, > > sorry for the delay. > > On Mar 08, Mark Hindley wrote: > > On Fri, Mar 07, 2014 at 05:17:30PM +0100, Uwe Storbeck wrote: > > > So we have 2 different problems, apt-get/aptitude on stable > > > and apt-get on testing/unstable throw the HTTP headers into > > > the changelog output when using apt-cacher and aptitude on > > > testing/unstable fails to get the changelog because it uses a > > > different URL than apt-get to fetch it? > > > > Could you test these 2 patches and see if they help? > > I've applied both patches and the patch from your 2nd mail to > apt-cacher 1.7.6 from stable. > > Both apt-get and aptitude work as expected now, when I switch off > the use of an external proxy. It looks like the primary issue of > this bug is fixed by your patches, aptitude changelog does not > fail anymore.
Good! > But when I use squid as an external proxy (use_proxy = 1) > apt-get from stable (version 0.9.7.9+deb7) and testing (version > 0.9.15.5+b1) and aptitude from stable (version 0.6.8.2-1), but > not aptitude from testing (version 0.6.10-1), still throw HTTP > headers into the changelog output when apt-cacher has to > retrieve the changelog file from the debian server (see attached > apt-cacher log). The output in apt-get or aptitude looks like > this: > > HTTP/1.0 200 Connection established > Connection: Keep-Alive > Date: Thu, 27 Mar 2014 19:56:32 GMT > Via: 1.1 c1:3142 (apt-cacher/1.7.6) > Accept-Ranges: bytes > Content-Length: 12381 > Keep-Alive: timeout=15, max=100 > X-AptCacher-URL: > http://packages.debian.org/changelogs/pool/main/e/ed/ed_1.10-2/changelog OK, could you try this patch please (on top of the others). Although I cannot reproduce this behavious (I think I have an upstream transparent proxy that is filtering all the redirects), I think I understand what was wrong and hope this addresses it. Although the patch looks big, it is mostly a refactoring of the logic. I think it should reliably read all the headers before going on to the body. Mark commit a6136d989260ccae4df148dd08d14af483d7b1e3 Author: Mark Hindley <m...@hindley.org.uk> Date: Mon Mar 31 08:16:27 2014 +0100 Really deal properly with multiple libcurl headers on redirects. Read all the headers, don't assume that 200 is the final one as we can get this as part of a HTTPS proxy CONNECT during a redirect sequence. diff --git a/apt-cacher b/apt-cacher index f4b58eb..f306cb3 100755 --- a/apt-cacher +++ b/apt-cacher @@ -1623,18 +1623,22 @@ sub libcurl { <$libcurl> }) { # $cfg->{debug} && debug_message('libcurl returned ' . length($_) . ' bytes'); + + # Handle libcurl status if (s/APT-CACHER_LIBCURL_STATUS=(\d+)\n$//) { # Match and remove, including newline $curl_status = $1; $curl_errbuf = WWW::Curl::Easy->new->strerror($curl_status); $cfg->{debug} && debug_message("Found EOF marker and status $curl_status ($curl_errbuf)"); - last if $curl_status; # Bail out if we get a curl error + last if $curl_status; # Bail out if we get a libcurl error # Otherwise go on to parse $_ as it will contain the file tail for binary files } + + # Handle headers if (!$response || m#^HTTP/1\.[01]\s+\d{3}\s+#) { info_message("Got another status line. Redirected?: $_") if $response; $response=HTTP::Response->parse($_); if ($response->code) { - $cfg->{debug} && debug_message('libcurl reading of headers complete: ' . $response->code); + $cfg->{debug} && debug_message('Successfully parsed header: ' . $response->code); chomp_message($response); # Handle chunked if ($response->header('Transfer-Encoding') && lc $response->header('Transfer-Encoding') eq 'chunked') { @@ -1644,105 +1648,109 @@ sub libcurl { $response->remove_header('Transfer-Encoding'); $response->remove_header('Content-Length'); } - - if ($curl_request->method eq 'GET' && $response->is_success) { - - # Check space - my $statfs; - if (defined($statfs = df($cfg->{'cache_dir'}, 1)) && - $response->header('Content-Length') && - $response->header('Content-Length') >= $statfs->{bavail} || - $cfg->{_disk_usage_limit} && - $statfs->{used} + $response->header('Content-Length') >= $cfg->{_disk_usage_limit}) { - info_message('ALARM: Insuffient space for Content-Length: '. - $response->header('Content-Length'). - ' in cache_dir with ' . $statfs->{bavail} . ' available space'); - $response=HTTP::Response->new(503, 'apt-cacher: Cache Full'); - $response->protocol('HTTP/1.1'); + next; + } + else { + info_message("Warning: failed to parse headers: $_"); + $response=HTTP::Response->new(502, 'apt-cacher: failed to parse headers'); + $response->protocol('HTTP/1.1'); + last; + } + } + + # Fork fetcher for successful GET requests, if not already done + if ($curl_request->method eq 'GET' && $response->is_success && !defined($fpid)) { + # Check space + my $statfs; + if (defined($statfs = df($cfg->{'cache_dir'}, 1)) && + $response->header('Content-Length') && + $response->header('Content-Length') >= $statfs->{bavail} || + $cfg->{_disk_usage_limit} && + $statfs->{used} + $response->header('Content-Length') >= $cfg->{_disk_usage_limit}) { + info_message('ALARM: Insuffient space for Content-Length: '. + $response->header('Content-Length'). + ' in cache_dir with ' . $statfs->{bavail} . ' available space'); + $response=HTTP::Response->new(503, 'apt-cacher: Cache Full'); + $response->protocol('HTTP/1.1'); + } + else { + # Take a lock on the target file and truncate + _flock($cache->{content}, LOCK_EX) || die 'Unable to lock the target file'; + $cache->{content}->truncate(0) || die "Truncate failed: $!"; + seek($cache->{content},0,0) || die "Seek failed: $!";; + + defined($fpid = fork()) || die "Fork fetcher failed: $!"; + if ($fpid) { + # Parent + $cfg->{debug} && debug_message("Forked fetcher $fpid"); + + # Reopen content filedescriptor + # separately. Relying on on dup(2) at fork + # leaves a shared seek pointer. + if (sysopen my $reopen, fd_path($cache->{content}), O_RDWR) { + $cache->{content} = $reopen; } else { - # Take a lock on the target file and truncate - _flock($cache->{content}, LOCK_EX) || die 'Unable to lock the target file'; - $cache->{content}->truncate(0) || die "Truncate failed: $!"; - seek($cache->{content},0,0) || die "Seek failed: $!";; - - defined($fpid = fork()) || die "Fork fetcher failed: $!"; - if ($fpid) { - # Parent - $cfg->{debug} && debug_message("Forked fetcher $fpid"); - - # Reopen content filedescriptor - # separately. Relying on on dup(2) at fork - # leaves a shared seek pointer. - if (sysopen my $reopen, fd_path($cache->{content}), O_RDWR) { - $cache->{content} = $reopen; - } - else { - die "Failed to reopen content: $!"; - } - - if ($curl_request->method eq 'GET') { - if (!defined $response->content_length) { - # HTTP/1.0 or HTTP/1.1 chunked server upstream - # Wait for fetcher - $cfg->{debug} && debug_message('No Content-Length received for '. $curl_request->uri . '. You may get better performance using a different upstream server.'); - _flock($cache->{content}, LOCK_SH); # Wait for the fetcher to release lock - $response->content_length(-s $cache->{content}); - } - - if($cfg->{checksum}) { - if (!is_file_type('installer', $request->uri->path) - && !is_file_type('skip_checksum', $request->uri->path)) { - # check for file corruption - _flock($cache->{content}, LOCK_SH); # Wait for the fetcher to release lock - my $cached_file = filename_fh($cache->{content}); - $cfg->{debug} && debug_message("Validating checksum for $cached_file"); - my $refreshed_index; - while (!check_sum($cache->{name}, $cache->{content})) { - if (is_file_type('index', $request->uri->path) && !$refreshed_index) { - # If an index file, refresh the Release file to update - # the checksum database and retry - info_message("Checksum mismatch on fetched $cached_file. Refreshing Release file"); - refresh_release($request->uri); - $refreshed_index = 1; - $cfg->{debug} && debug_message("Revalidating checksum for $cached_file"); - next; - } - info_message("ALARM! checksum mismatch on $cached_file"); - kill 15, $fpid; # Kill the fetcher as it isn't doing anything useful! - _flock($cache->{content}, LOCK_UN); # Not interested in this any more but leave filesystem unlink to fetch_store() - $response = HTTP::Response->new(502, 'Data corruption'); - $response->protocol('HTTP/1.1'); - last; - } - } + die "Failed to reopen content: $!"; + } + + if ($curl_request->method eq 'GET') { + if (!defined $response->content_length) { + # HTTP/1.0 or HTTP/1.1 chunked server upstream + # Wait for fetcher + $cfg->{debug} && debug_message('No Content-Length received for '. $curl_request->uri . '. You may get better performance using a different upstream server.'); + _flock($cache->{content}, LOCK_SH); # Wait for the fetcher to release lock + $response->content_length(-s $cache->{content}); + } - # For internal requests wait for the fetcher to finish completely - # so that import_sums() has run - if ($cache->{internal}) { - debug_message("Waiting for $fpid"); - waitpid($fpid,0); + if($cfg->{checksum}) { + if (!is_file_type('installer', $request->uri->path) + && !is_file_type('skip_checksum', $request->uri->path)) { + # check for file corruption + _flock($cache->{content}, LOCK_SH); # Wait for the fetcher to release lock + my $cached_file = filename_fh($cache->{content}); + $cfg->{debug} && debug_message("Validating checksum for $cached_file"); + my $refreshed_index; + while (!check_sum($cache->{name}, $cache->{content})) { + if (is_file_type('index', $request->uri->path) && !$refreshed_index) { + # If an index file, refresh the Release file to update + # the checksum database and retry + info_message("Checksum mismatch on fetched $cached_file. Refreshing Release file"); + refresh_release($request->uri); + $refreshed_index = 1; + $cfg->{debug} && debug_message("Revalidating checksum for $cached_file"); + next; } + info_message("ALARM! checksum mismatch on $cached_file"); + kill 15, $fpid; # Kill the fetcher as it isn't doing anything useful! + _flock($cache->{content}, LOCK_UN); # Not interested in this any more but leave filesystem unlink to fetch_store() + $response = HTTP::Response->new(502, 'Data corruption'); + $response->protocol('HTTP/1.1'); + last; } } - last; # <$libcurl> - } - else { - # Child continues to fetch - setpgrp(0,0); # New process group - $0="$0 [${\$request->uri}]"; # Visible to ps and friends, not local - $cache->{content}->autoflush; + + # For internal requests wait for the fetcher to finish completely + # so that import_sums() has run + if ($cache->{internal}) { + debug_message("Waiting for $fpid"); + waitpid($fpid,0); + } } } + last; # <$libcurl> + } + else { + # Child continues to fetch + setpgrp(0,0); # New process group + $0="$0 [${\$request->uri}]"; # Visible to ps and friends, not local + $cache->{content}->autoflush; } - } - else { - info_message("Warning: failed to parse headers: $_"); - $response=HTTP::Response->new(502, 'apt-cacher: failed to parse headers'); - $response->protocol('HTTP/1.1'); } } - elsif ($curl_request->method eq 'POST') { + + # Handle body + if ($curl_request->method eq 'POST') { $response->add_content($_); } elsif ($cache->{content}) { @@ -1753,6 +1761,7 @@ sub libcurl { } } } + if (!$response) { if (!defined $curl_status) { $curl_status=1; -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
