Skip to content

fix(http_client): don't hold mutex_ across the OnTcpData backpressure wait - #53

Open
max-power wants to merge 1 commit into
78:mainfrom
max-power:fix/http-client-ontcpdata-deadlock
Open

fix(http_client): don't hold mutex_ across the OnTcpData backpressure wait#53
max-power wants to merge 1 commit into
78:mainfrom
max-power:fix/http-client-ontcpdata-deadlock

Conversation

@max-power

Copy link
Copy Markdown

Summary

  • HttpClient::OnTcpData() acquires mutex_ for the entire function, including while it waits on write_cv_ for body_chunks_ to drain (or for connected_ to become false).
  • connected_ can only be set to false by HttpClient::OnTcpDisconnected() — which itself needs to acquire that same mutex_ first.
  • If the TCP receive task is inside that wait when the connection drops, OnTcpDisconnected() (called from EspTcp's disconnect path, typically from the receive task itself or a related callback) can never acquire mutex_ to set connected_ = false, which is exactly the condition the wait needs to unblock. The two deadlock permanently.
  • On a dual-core target this doesn't just hang the one task — a task spinning forever inside the critical section a mutex unlock/notify enters can stall the shared cross-core spinlock, which can starve the other core's unrelated interrupt handling too, eventually tripping the interrupt watchdog and panicking the whole device.

Neither the write_cv_ wait predicate nor OnTcpDisconnected() ever touches anything mutex_ protects — the predicate only reads read_mutex_-protected body_chunks_ and connected_, and OnTcpDisconnected() never touches body_chunks_/read_mutex_/write_cv_ at all. So narrowing mutex_'s scope to just the rx_buffer_.append()/ProcessReceivedData() call after the wait doesn't change any protected invariant — it just stops holding an unrelated lock across a blocking wait.

Reproduction conditions: most likely to surface with a large/slow upload where the server responds with an error and closes the connection mid-transfer. A fast, small request usually completes before body_chunks_ ever grows enough to need throttling, so the wait — and the deadlock — never triggers. We hit this in practice uploading a large multipart image upload that got rejected mid-transfer by the server; the crash's backtrace symptom varied between occurrences (different apparent fault locations), consistent with a system-wide interrupt-watchdog stall rather than a clean, single-location crash.

Test plan

  • Applied this exact change against the vendored copy in a downstream ESP-IDF project (xiaozhi-esp32, ESP32-S3), rebuilt, and confirmed the project still builds and runs normally (HTTP requests, uploads, etc. all continue to work).
  • Have not built a minimal standalone reproduction outside that project — this repo doesn't have a test app to reproduce the race in isolation, and the trigger is timing-dependent (needs a slow/large upload plus a server that aborts mid-transfer).

… wait

OnTcpData() acquired mutex_ for the whole function, including while
waiting on write_cv_ for body_chunks_ to drain (or for connected_ to
become false). But connected_ can only be set by OnTcpDisconnected(),
which itself needs to acquire mutex_ first — so if the receive task is
blocked in that wait when the connection drops, OnTcpDisconnected()
can never acquire the lock to update connected_, and the two calls
deadlock permanently (the receive task spins forever inside the
critical section a mutex unlock/notify enters, which can starve the
other core's own interrupt handling and eventually trip the interrupt
watchdog).

Neither the wait predicate nor OnTcpDisconnected() ever touches
anything mutex_ protects — the predicate only reads read_mutex_-
protected body_chunks_ and connected_, and OnTcpDisconnected() never
touches body_chunks_/read_mutex_ at all — so narrowing mutex_'s scope
to just the rx_buffer_/ProcessReceivedData() call after the wait
doesn't change any protected invariant, it just stops holding an
unrelated lock across a blocking wait.

Most likely to surface with a large/slow upload where the server
aborts the connection mid-transfer (a fast small request completes
before body_chunks_ ever needs to throttle, so the wait — and the
deadlock — never triggers).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant