Skip to content

Fix HttpAsyncHook crash on stream/cert/trust_env connection extras - #71524

Open
gtxu wants to merge 1 commit into
apache:mainfrom
gtxu:fix-http-async-hook-extra-options
Open

Fix HttpAsyncHook crash on stream/cert/trust_env connection extras#71524
gtxu wants to merge 1 commit into
apache:mainfrom
gtxu:fix-http-async-hook-extra-options

Conversation

@gtxu

@gtxu gtxu commented Aug 13, 2026

Copy link
Copy Markdown

HttpAsyncHook/AsyncHttpSession.run() shares _process_extra_options_from_connection() with the sync HttpHook, but that function's output uses requests-flavored option names, and the async path blindly forwards all of them as kwargs into aiohttp.ClientSession's request call. aiohttp doesn't accept stream, cert, or trust_env as request kwargs at all, so any HTTP connection with one of those three keys set in its Extra field — or passed via extra_options= at call time — crashed every single request with TypeError: request() got an unexpected keyword argument '...'. This affects both direct HttpAsyncHook usage and Airflow's deferred/async HTTP triggers, since they route through the same code path.

Separately, check_response (meant to suppress raise_for_status() on non-2xx responses, matching sync's run_and_check()) was accepted into the same shared dict but never actually consumed on the async side — AsyncHttpSession.run() always called raise_for_status() unconditionally, so setting check_response: false had no effect on async requests.

Fix:

  • AsyncHttpSession.run() now pops check_response and honors it before calling raise_for_status(), bringing async to parity with sync's existing behavior.
  • stream, cert, and trust_env are filtered out before the aiohttp call, with a warning logged listing which were dropped. These three have no aiohttp equivalent worth implementing here: aiohttp streams responses by default (no stream flag needed), and cert/trust_env would require real work (building an ssl.SSLContext, restructuring session construction) that's out of scope for a crash fix.
  • Corrected an existing test that had been asserting trust_env gets passed through to the aiohttp mock — it only passed because the plain AsyncMock doesn't validate kwargs against aiohttp's real method signature, which is exactly how this bug went undetected.

Test plan:

  • Added regression tests confirming stream/cert/trust_env never reach the aiohttp call (with the warning logged) and that check_response: false suppresses raise_for_status() on async requests.
  • Full providers-tests --test-type "Providers[http]" suite, mypy, and ruff all pass.

Drafted-by: Claude Code (Sonnet 5) Reviewed-by: @gtxu before submission


Was generative AI tooling used to co-author this PR?
  • Yes Claude Code (Sonnet 5)

Generated-by: Claude Code (Sonnet 5) following the guidelines


@gtxu
gtxu force-pushed the fix-http-async-hook-extra-options branch from 12f9ae6 to 917840d Compare August 13, 2026 01:09
@gtxu
gtxu marked this pull request as ready for review August 13, 2026 01:10
HttpAsyncHook shares its extra-options parsing with the synchronous
HttpHook, but the shared helper's output is requests-flavored and gets
forwarded as-is into aiohttp's request call. aiohttp rejects stream,
cert, and trust_env as unexpected keyword arguments, so any HTTP
connection carrying one of those keys in its Extra field crashed every
async request. check_response was accepted into the same dict but
never actually consumed on the async side, so it silently had no
effect unlike the synchronous path's run_and_check().
@gtxu
gtxu force-pushed the fix-http-async-hook-extra-options branch from 917840d to 6de7bc3 Compare August 13, 2026 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant