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

[Scott Mitchell]

Thanks for the additional details :)

Agreed there is ambiguity in the RFC for error codes, and retryability will require additional context either way (idempotent, safe, ..). Some points to consider:
- PROTOCOL_ERROR is allowed as a “catch all”, but the client can’t differentiate from other “real protocol errors” (e.g. incorrect frame format, sequencing, ..) which generally aren’t retryable.
- CANCEL also doesn’t tell the client “why”, but it is known to be application level (as opposed to a protocol issue)

wdyt? Since there is no “right answer” understood if this is deemed undesirable or too risky.

-Scott

> On May 2, 2022, at 4:14 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> 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/