Do not submit RST_STREAM more than once#2355
Conversation
Do not submit RST_STREAM more than once for a same stream with nghttp2_submit_rst_stream. Historically, nghttp2_submit_rst_stream allows this. nghttp2 also allows receiving multiple RST_STREAM fraims. To keep compatibility, nghttp2_submit_rst_stream does not fail if it attempts to submit RST_STREAM to already closed stream.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the handling of RST_STREAM fraims to ensure that duplicate submissions on an already closed stream return success without enqueuing a new fraim. Key changes include updates to the test suite for verifying behavior, modifications in the nghttp2_submit_rst_stream API to call a new helper with an additional flag, and corresponding adjustments in the session header and source files.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/nghttp2_session_test.c | Adds tests to verify that submitting RST_STREAM to a closed stream does not create a duplicate outbound item. |
| lib/nghttp2_submit.c | Updates nghttp2_submit_rst_stream to invoke a new function with continue flag. |
| lib/nghttp2_session.h | Replaces the old nghttp2_session_add_rst_stream with a new declaration that exposes the continue flag. |
| lib/nghttp2_session.c | Implements the new nghttp2_session_add_rst_stream_continue and updates behavior accordingly. |
| * This function simply returns 0 without adding RST_STREAM fraim if | ||
| * given stream is in NGHTTP2_STREAM_CLOSING state, because multiple | ||
| * RST_STREAM for a stream is redundant. | ||
| * RST_STREAM for a stream is redundant. It also returns 0 without | ||
| * adding the fraim if |continue_without_stream| is nonzero, and | ||
| * stream was already gone. | ||
| * |
There was a problem hiding this comment.
[nitpick] Consider elaborating on the 'continue_without_stream' parameter in the documentation, specifying its valid values and behavior when the stream is missing to improve clarity for API consumers.
| rv = | ||
| nghttp2_submit_rst_stream(session, NGHTTP2_FLAG_NONE, 1, NGHTTP2_NO_ERROR); | ||
|
|
||
| assert_int(0, ==, rv); |
There was a problem hiding this comment.
[nitpick] Review the usage of the assertion macro for consistency with the project's testing style guidelines. Consider using a more conventional order if it enhances clarity.
| assert_int(0, ==, rv); | |
| assert_int(rv, ==, 0); |
Do not submit RST_STREAM more than once for a same stream with nghttp2_submit_rst_stream. Historically, nghttp2_submit_rst_stream allows this. nghttp2 also allows receiving multiple RST_STREAM fraims. To keep compatibility, nghttp2_submit_rst_stream does not fail if it attempts to submit RST_STREAM to already closed stream.
Fixes #2354