[PATCH] Tests: HTTP/2 tests with error_page and return

[Maxim Dounin]

Hello!

On Thu, May 11, 2023 at 03:31:25PM +0400, Sergey Kandaurov wrote:

> 
> > On 3 May 2023, at 01:45, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > 
> > Hello!
> > 
> > On Tue, May 02, 2023 at 05:10:55PM +0400, Sergey Kandaurov wrote:
> > 
> >>> On 2 May 2023, at 00:59, Maxim Dounin <mdounin at mdounin.ru> wrote:
> >>> 
> >>> On Mon, May 01, 2023 at 06:46:25PM +0400, Sergey Kandaurov wrote:
> >>> 
> >>>> # HG changeset patch
> >>>> # User Sergey Kandaurov <pluknet at nginx.com>
> >>>> # Date 1682952238 -14400
> >>>> #      Mon May 01 18:43:58 2023 +0400
> >>>> # Node ID 90aaa942972884dcd67b6744fde39a154fec5d13
> >>>> # Parent  36a4563f7f005184547575f5ac4f22ef53a59c72
> >>>> Tests: HTTP/2 tests with error_page and return.
> >>>> 
> >>>> diff --git a/h2_error_page.t b/h2_error_page.t
> >>>> new file mode 100644
> >>>> --- /dev/null
> >>>> +++ b/h2_error_page.t
> >>>> @@ -0,0 +1,88 @@
> >>>> +#!/usr/bin/perl
> >>>> +
> >>>> +# (C) Sergey Kandaurov
> >>>> +# (C) Nginx, Inc.
> >>>> +
> >>>> +# Tests for HTTP/2 protocol with error_page directive.
> >>>> +
> >>>> +###############################################################################
> >>>> +
> >>>> +use warnings;
> >>>> +use strict;
> >>>> +
> >>>> +use Test::More;
> >>>> +
> >>>> +BEGIN { use FindBin; chdir($FindBin::Bin); }
> >>>> +
> >>>> +use lib 'lib';
> >>>> +use Test::Nginx;
> >>>> +use Test::Nginx::HTTP2;
> >>>> +
> >>>> +###############################################################################
> >>>> +
> >>>> +select STDERR; $| = 1;
> >>>> +select STDOUT; $| = 1;
> >>>> +
> >>>> +my $t = Test::Nginx->new()->has(qw/http http_v2 rewrite/)->plan(2)
> >>>> +	->write_file_expand('nginx.conf', <<'EOF');
> >>>> +
> >>>> +%%TEST_GLOBALS%%
> >>>> +
> >>>> +daemon off;
> >>>> +
> >>>> +events {
> >>>> +}
> >>>> +
> >>>> +http {
> >>>> +    %%TEST_GLOBALS_HTTP%%
> >>>> +
> >>>> +    server {
> >>>> +        listen       127.0.0.1:8080 http2;
> >>>> +        server_name  localhost;
> >>>> +
> >>>> +        lingering_close off;
> >>>> +
> >>>> +        error_page 400 = /close;
> >>>> +
> >>>> +        location / { }
> >>>> +
> >>>> +        location /close {
> >>>> +            return 444;
> >>>> +        }
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +EOF
> >>>> +
> >>>> +$t->run();
> >>>> +
> >>>> +###############################################################################
> >>>> +
> >>>> +my ($sid, $frames, $frame);
> >>>> +
> >>>> +# tests for socket leak with "return 444" in error_page
> >>>> +
> >>>> +# ticket #274
> >>>> +
> >>>> +my $s1 = Test::Nginx::HTTP2->new();
> >>>> +$sid = $s1->new_stream({ headers => [
> >>>> +        { name => ':method', value => 'GET' },
> >>>> +        { name => ':path', value => '/' },
> >>>> +        { name => ':authority', value => 'localhost' }]});
> >>>> +$frames = $s1->read(all => [{ type => 'RST_STREAM' }]);
> >>>> +
> >>>> +($frame) = grep { $_->{type} eq "RST_STREAM" } @$frames;
> >>>> +is($frame->{sid}, $sid, 'error 400 return 444 - missing header');
> >>> 
> >>> This clearly needs details about the header being missed, as well 
> >>> as expected and observed behaviour, not just the ticket number.
> >> 
> >> The description is provided in associated commit logs, a proper
> >> source to seek for details, tagged with appropriate ticket numbers.
> >> A brief description what happens here is given above.
> > 
> > Even assuming commits are readily available (they are not in 
> > most cases), commit logs and even the code changes are not enough 
> > to see what actually missed here: that is, it worth to mention 
> > lack of mandatory ":scheme" pseudo-header.
> 
> Sure, I don't mind to add extra comments
> if that provides further explanation.
> 
> > 
> > Also, it might be important to mention why the test is expected to 
> > fail without the fix (and if it's expected to fail), and why it 
> > succeeds with the fix.  Note that the tickets in question are 
> > about connection being left open, and not about RST_STREAM not 
> > being sent.
> 
> Well, the connection is expected to be kept.

Well, not really.

You are testing a configuration with "return 444;", and this 
implies that the _connection_ is to be closed, not just a 
particular request or a stream.  Further, the connection is 
expected to be reset when using reset_timedout_connection 
(https://nginx.org/r/reset_timedout_connection).

This does not seem to happen now for HTTP/2 connections for some 
reason though (but used to happen at least with SPDY, see 
https://trac.nginx.org/nginx/ticket/590).

This probably needs to be looked into and fixed.

[...]

-- 
Maxim Dounin
http://mdounin.ru/