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

[Maxim Dounin]

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.

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.

Note well that RST_STREAM is not something one might expect with 
"return 444;", and rather an implementation detail.  A better 
approach might be to check instead for a connection being closed 
and not timed out on the client side (this might complicate things 
though, and might not worth the effort).

> If you insist, we can add further details inline:
> 
> # a socket leak observed on missing ngx_http_run_posted_requests() such as
> # in ngx_http_v2_run_request(), e.g. if ngx_http_v2_construct_request_line()
> # failed due to missing mandatory ":scheme" pseudo-header (ticket #274)

These are indeed mostly implementation details.  A better comment 
might be:

# make sure there is no socket leak when the request is rejected
# due to missing mandatory ":scheme" pseudo-header and "return 444;"
# is used in error_page 400 (ticket #274)

> > 
> >> +
> >> +# ticket #2455
> >> +
> >> +my $s2 = Test::Nginx::HTTP2->new();
> >> +$sid = $s2->new_stream({ method => 'foo' });
> >> +$frames = $s2->read(all => [{ type => 'RST_STREAM' }]);
> >> +
> >> +($frame) = grep { $_->{type} eq "RST_STREAM" } @$frames;
> >> +is($frame->{sid}, $sid, 'error 400 return 444 - invalid header');
> > 
> > Same here.
> 
> # another case with missing ngx_http_run_posted_requests() is in
> # ngx_http_v2_state_process_header() error handling (ticket #2455),
> # can be triggered with invalid pseudo-header

Same here:

# make sure there is no socket leak when the request is rejected
# due to invalid method with lower-case letters and "return 444;"
# is used in error_page 400 (ticket #2455)

> 
> > 
> >> +
> >> +$t->stop();

This also might worth an explicit comment.  Something like

# while keeping $s1 and $s2, stop nginx; this should result in
# "open socket ... left in connection ..." alerts if any of these
# sockets is still open

should be good enough.

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