Skip to content

http2: changes in error handling - #22870

Closed
icing wants to merge 2 commits into
curl:masterfrom
icing:h2-error-handling-fix
Closed

icing wants to merge 2 commits into
curl:masterfrom
icing:h2-error-handling-fix

Conversation

@icing

@icing icing commented Sep 8, 2026

Copy link
Copy Markdown
Contributor
  • always treat errors from nghttp2_session_mem_recv() as fatal
  • always return 0 from invalid_frame callback, so that nghttp2's internal RST/GOAWAY handling can work
  • adjust test_01_14 return code expectation

- always treat errors from nghttp2_session_mem_recv() as fatal
- always return 0 from invalid_frame callback, so that nghttp2's
  internal RST/GOAWAY handling can work
- adjust test_01_14 return code expectation
@icing

icing commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@vszakats the Windows errors are caused by some cmake related change, maybe?

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes core HTTP/2 error/callback semantics with nghttp2, which can have subtle protocol/connection behavior impacts that should be validated by a maintainer with full test coverage.

Pull request overview

This PR adjusts libcurl’s HTTP/2 (nghttp2) error-handling behavior to treat nghttp2_session_mem_recv() errors as fatal, to always return success from the invalid_frame callback so nghttp2 can perform its own RST/GOAWAY handling, and aligns an HTTP/2 test expectation with the resulting exit code.

Changes:

  • Change the nghttp2_session_mem_recv() error path in h2_process_pending_input() to return CURLE_RECV_ERROR.
  • Make cf_h2_on_invalid_frame_recv() always return 0 (success) instead of NGHTTP2_ERR_CALLBACK_FAILURE.
  • Update test_01_14_gigalarge_resp_headers to expect exit code 56 (CURLE_RECV_ERROR) for HTTP/2.
File summaries
File Description
lib/http2.c Updates HTTP/2 receive/error callback handling to change fatality and allow nghttp2 internal recovery signaling.
tests/http/test_01_basic.py Updates HTTP/2 test expectation to match the new error code surfaced by the changed receive path.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/http2.c Outdated
@vszakats

vszakats commented Sep 8, 2026

Copy link
Copy Markdown
Member

@vszakats the Windows errors are caused by some cmake related change, maybe?

The Cygwin fail seen today looks like upstream temporary breakage. Let's see if it resolves itself.

@icing

icing commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@vszakats the Windows errors are caused by some cmake related change, maybe?

The Cygwin fail seen today looks like upstream temporary breakage. Let's see if it resolves itself.

It did. Thanks for checking.

@icing
icing requested a review from bagder September 8, 2026 09:45
@bagder bagder closed this in f86897d Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants