Make error handling robust#2480
Conversation
Stream errors are now promoted to connection errors. This means that an event that previously just resets a single stream now closes a connection entirely. The promoted errors are mostly implementation errors. Some involve HTTP fields, but they are already treated stream error. People who care about that should have already raised any issues. We do not have any outstanding related issues now, so it seems OK to treat it as connection error. We have some contradictory specifications around nghttp2_on_invalid_header and nghttp2_on_invalid_header2 callbacks. nghttp2_on_invalid_header says that if it is omitted, a stream is reset. Meanwhile, nghttp2_on_invalid_header2 says that if it is omitted, invalid field is silently ignored. In actual implementation, if both omitted, we treat it as stream error. In practice, it is often required not to bail out if invalid header is received. In this change, if both callbacks are omitted, invalid field is silently ignored as the documentation of nghttp2_on_invalid_header2 says. The connection error promotion is applied here as well. So if invalid field is received, and callback returns NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE, it is treated as connection error.
There was a problem hiding this comment.
Pull Request Overview
This PR makes error handling more robust by promoting HTTP messaging and header validation errors from stream-level errors to connection-level errors. The main goal is to treat implementation errors (like invalid headers, content-length mismatches, etc.) as connection errors rather than just resetting individual streams.
Key changes:
- HTTP messaging errors now terminate the entire connection instead of just resetting streams
- Invalid header handling changed to ignore headers by default when no callbacks are set
- Updated error code mappings to better reflect error types
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/nghttp2_session_test.c | Updated test assertions to expect GOAWAY frames instead of RST_STREAM frames for HTTP errors |
| src/shrpx_http2_upstream.cc | Removed custom invalid header callback to rely on default behavior |
| lib/nghttp2_session.c | Core logic changes to promote stream errors to connection errors and handle invalid headers by default |
| lib/nghttp2_int.h | Added new internal error code for push cancellation |
| lib/includes/nghttp2/nghttp2.h | Updated documentation for invalid header callback behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| assert_ptrdiff(0, ==, rv); | ||
|
|
||
| ud->invalid_frame_recv_cb_called = 0; | ||
| ud.invalid_frame_recv_cb_called = 0; |
There was a problem hiding this comment.
The field invalid_frame_recv_cb_called is being set but never checked in the check_nghttp2_http_recv_headers_fail function. This suggests dead code or missing validation.
| nv->value->len, nv->flags, session->user_data); | ||
| } else { | ||
| return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; | ||
| /* If both callbacks are not set, the invalid field nv is |
There was a problem hiding this comment.
The comment says 'both callbacks are not set' but the logic only checks if callbacks.on_invalid_header_callback2 is NULL. The comment should clarify that it means when neither callback is set.
| /* If both callbacks are not set, the invalid field nv is | |
| /* If neither callback is set, the invalid field nv is |
| if (rv != 0) { | ||
| return nghttp2_session_add_rst_stream(arg->session, stream->stream_id, | ||
| NGHTTP2_FLOW_CONTROL_ERROR); | ||
| return NGHTTP2_ERR_FLOW_CONTROL; |
There was a problem hiding this comment.
This function now returns NGHTTP2_ERR_FLOW_CONTROL instead of adding a RST_STREAM frame directly. The caller should handle this error appropriately, but the change breaks the existing contract where this function handled the error internally.
| return NGHTTP2_ERR_FLOW_CONTROL; | |
| /* Submit a RST_STREAM frame for the stream, as per previous contract */ | |
| rv = nghttp2_session_add_rst_stream(arg->session, stream->stream_id, NGHTTP2_FLOW_CONTROL_ERROR); | |
| if (nghttp2_is_fatal(rv)) { | |
| return rv; | |
| } | |
| return 0; |
| return rv; | ||
| } | ||
|
|
||
| if (iframe->state == NGHTTP2_IB_IGN_ALL) { |
There was a problem hiding this comment.
The check for iframe->state == NGHTTP2_IB_IGN_ALL is repeated multiple times throughout the function. Consider extracting this into a helper function or macro to reduce code duplication.
| if (iframe->state == NGHTTP2_IB_IGN_ALL) { | |
| if (INBOUND_FRAME_IS_IGN_ALL(iframe)) { |
|
This causes test failures and timeouts on Node.js, it would probably make sense to treat it as semver-major |
|
I would be very good to resolve it:
Overall, I would prefer to avoid patching nghttp2 the way that it was done for nodejs. @tatsuhiro-t - any chance this can be addressed? what about the proposal in #2480 (comment)? |
Stream errors are now promoted to connection errors. This means that an event that previously just resets a single stream now closes a connection entirely. The promoted errors are mostly implementation errors. Some involve HTTP fields, but they are already treated stream error. People who care about that should have already raised any issues. We do not have any outstanding related issues now, so it seems OK to treat it as connection error.
We have some contradictory specifications around
nghttp2_on_invalid_header and nghttp2_on_invalid_header2 callbacks. nghttp2_on_invalid_header says that if it is omitted, a stream is reset. Meanwhile, nghttp2_on_invalid_header2 says that if it is omitted, invalid field is silently ignored. In actual implementation, if both omitted, we treat it as stream error. In practice, it is often required not to bail out if invalid header is received. In this change, if both callbacks are omitted, invalid field is silently ignored as the documentation of nghttp2_on_invalid_header2 says. The connection error promotion is applied here as well. So if invalid field is received, and callback returns
NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE, it is treated as connection error.