Hello!

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

>
> > On 3 May 2023, at 01:45, Maxim Dounin <mdounin@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@mdounin.ru> wrote:
> >>>
> >>> On Mon, May 01, 2023 at 06:46:25PM +0400, Sergey Kandaurov wrote:
> >>>
> >>>> # HG changeset patch
> >>>> # User Sergey Kandaurov <pluknet@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel