Proposal: Change H2 RST_STREAM error code on timeout PROTOCOL_ERROR->CANCEL

[Maxim Dounin]

Hello!

On Mon, May 02, 2022 at 08:19:44AM -0700, Scott Mitchell via nginx-devel wrote:

> Rational is captured in the patch set below (tests also 
> updated). Please let me know if this is acceptable.
> 

> # HG changeset patch
> # User Scott Mitchell <scott_mitchell at apple.com>
> # Date 1651280176 25200
> #      Fri Apr 29 17:56:16 2022 -0700
> # Node ID ac2a48aa4efb86cd34189dd1b62fe24c5afbab70
> # Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
> H2: RST_STREAM(CANCEL) on timeout instead of PROTOCOL_ERROR
> 
> When a timeout occurs this condition maybe temporary and retryable. For example if the network
> bandwidth is temporarily throttled. If a PROTOCOL_ERROR is returned the client is unlikely to
> retry due to unknown error conditions. Returning CANCEL provides more context as an application
> level error that the client is more likely to retry.
> 
> diff -r a736a7a613ea -r ac2a48aa4efb src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c	Tue Feb 08 17:35:27 2022 +0300
> +++ b/src/http/v2/ngx_http_v2.c	Fri Apr 29 17:56:16 2022 -0700
> @@ -4636,7 +4636,7 @@
>  
>          if (!stream->out_closed) {
>              if (ngx_http_v2_send_rst_stream(h2c, node->id,
> -                                      fc->timedout ? NGX_HTTP_V2_PROTOCOL_ERROR
> +                                      fc->timedout ? NGX_HTTP_V2_CANCEL
>                                                     : NGX_HTTP_V2_INTERNAL_ERROR)
>                  != NGX_OK)
>              {

Thanks for the patch.

The CANCEL error code is defined by RFC 7540 as follows:

   CANCEL (0x8):  Used by the endpoint to indicate that the stream is no
      longer needed.

This does not seem to apply to the timeout case. In contrast, 
PROTOCOL_ERROR mostly matches the particular error condition (that 
is, client failed to provide further frames as expected per 
protocol in a reasonable time), and it is also defined to be a 
"catch-all" error:

   PROTOCOL_ERROR (0x1):  The endpoint detected an unspecific protocol
      error.  This error is for use when a more specific error code is
      not available.

As such, PROTOCOL_ERROR looks more appropriate in the particular 
case from the protocol point of view.

If the particular error condition is temporary and retry is 
desired, it is probably something to do per user action, and the 
particular error code shouldn't matter as long as retry is 
requested by the user.

Hope this helps.

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