Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19705 +/- ##
==========================================
+ Coverage 98.55% 98.59% +0.03%
==========================================
Files 182 182
Lines 23346 23346
Branches 5 5
==========================================
+ Hits 23009 23017 +8
+ Misses 336 328 -8
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Code size report: |
|
@dpgeorge, thank you for the review. Both comments are taken:
The halving retry is gone too: with Nagle on, Prior art, read from source: lwIP's own netconn returns PR reworked, three commits:
Commit 1 stands alone. If a narrower PR is preferred, commits 2 and 3 can move to their own PRs. TestOne script reproduces all three on any
Phase 1 passes when no non-blocking What was run: an OpenMV RT1062 (cyw43 Wi-Fi) on OpenMV's fork at MicroPython v1.28.0-49, where the retry loop waits in
|
lwip_tcp_send sizes a write from tcp_sndbuf, a byte count, but tcp_write allocates its segments from the heap and the MEMP_TCP_SEG pool and returns ERR_MEM when those run out while the count still shows room. The retry loop then waited up to 10s, for a non-blocking socket too. tcp_write queues nothing on ERR_MEM, so a non-blocking socket now returns EAGAIN from that loop, as it already does when tcp_sndbuf is 0. Blocking sockets are unchanged. Fixes issue micropython#19704. Signed-off-by: srgg <[email protected]>
9b4b80c to
e0dfeb3
Compare
The tcp_sndbuf==0 wait in lwip_tcp_send ends with ETIMEDOUT at the socket's timeout, but the ERR_MEM retry loop ended only when the write succeeded or 10s had passed, so a send on a socket with settimeout(t) could block far past t. Measured on an OpenMV RT1062 with t=0.5 and two slow readers: sends of 1651ms to 3401ms, none raising ETIMEDOUT. The loop now ends with ETIMEDOUT at the socket's timeout, counted from the start of the call so the tcp_sndbuf wait and the loop share one clock, and the 10s limit applies to a socket with no timeout only. Same test: every starved send raises ETIMEDOUT within 504ms. Fixes issue micropython#19746. Signed-off-by: srgg <[email protected]>
e0dfeb3 to
a17b43b
Compare
POLLOUT reads tcp_sndbuf, a byte count, so after an ERR_MEM EAGAIN a non-blocking socket polls writable and its next send gets EAGAIN again. lwIP's sockets gate on per-pcb low-water marks (NETCONN_FLAG_CHECK_WRITESPACE), which read one socket, while the memory ERR_MEM lacks is shared by all of them. After such an EAGAIN, POLLOUT now needs one segment's memory to be allocatable: a pbuf of mss bytes and one tcp_seg, both tried and released, plus a free slot in snd_queuelen. Since tcp_write queues all of write_len or nothing, the send retries once at a single mss before EAGAIN, so a writable poll is followed by an accepted write. Measured on an OpenMV RT1062 with two slow readers: 1295 EAGAINs, none right after POLLOUT. Signed-off-by: srgg <[email protected]>
a17b43b to
84847f9
Compare
Right. This PR is now quite complicated and requires some effort to understand its impact. Considering #19708 independently did exactly the same thing as Commit 1 here, I suggest we just merge that fix for now. And then follow up with a separate PR for the other changes. |
|
I understand the concern. Commit 1 contains the original fix I needed, while Commits 2 and 3 address additional issues I discovered along the way. I’d prefer to keep the changes together rather than spend time restructuring the PR. That said, if you’d prefer to keep this PR focused on the original fix, I’m fine with merging #19708 independently and leaving the other changes as they are for now. |
Resolves #19704, resolves #19746.
Summary
A non-blocking
socket.write()blocks for up to 10 s onERR_MEM(analysis in #19704).This change makes that path write the prefix that fits — halving
write_lenand retrying — and returnENOBUFSonly when nothing fits, instead of blocking.EAGAINis not used:POLLOUTreadstcp_sndbuf, so it would busy-spin aselectcaller;ENOBUFSis a resource error, not would-block. Blocking sockets are unchanged.Testing
Measured on an OpenMV RT1062 (cyw43 Wi-Fi) streaming MJPEG to a reader throttled to 20 KB/s, on a v1.28.0-based tree: worst
write()1553086 us, 54 events over 100 ms per 320 s; the early return removes both.Not built against
masterand run on no port here — the added block copies the adjacenttcp_sndbuf == 0return, and CI compiles theMICROPY_PY_LWIPports.Trade-offs and Alternatives
Halving costs up to log2(
tcp_sndbuf)tcp_writeattempts on the exhausted pass; returningENOBUFSwith zero bytes is O(1) but makes no progress.Generative AI
I used generative AI tools when creating this PR, but a human has checked the code and is responsible for the code and the description above.