> On 3 May 2023, at 01:45, Maxim Dounin <mdou...@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 <mdou...@mdounin.ru> wrote:
>>>
>>> On Mon, May 01, 2023 at 06:46:25PM +0400, Sergey Kandaurov wrote:
>>>
>>>> # HG changeset patch
>>>> # User Sergey Kandaurov <pluk...@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.
Unlike in HTTP/1.x, HTTP/2 is a multiplexing protocol.
That means an error condition in an individual stream doesn't lead
to the entire connection close. Rather, malformed requests lead
to a stream error indicated with RST_STREAM.
Let's look at RFC 7540:
5.4. Error Handling
HTTP/2 framing permits two classes of error:
o An error condition that renders the entire connection unusable is
a connection error.
o An error in an individual stream is a stream error.
So either GOAWAY or RST_STREAM is expected.
8.1.2.6. Malformed Requests and Responses
Malformed requests or responses that are
detected MUST be treated as a stream error (Section 5.4.2) of type
PROTOCOL_ERROR.
This is exactly what happens in nginx: it sends RST_STREAM
as an indication of immediate termination of a stream.
>
> Note well that RST_STREAM is not something one might expect with
> "return 444;", and rather an implementation detail.
See above, thanks.
> 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).
The problem is that stream termination didn't happen.
As such, stream count never fall below 1, which prevented
to install a keepalive timer. So, when it's time to shutdown
a worker process, there are no timers left in connection.
>
>> 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)
>
Thank you, much appreciated.
Applied to the changeset.
>>
>>>
>>>> +
>>>> +$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.
>
Thanks for the review, pushed.
--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
