This is an automated email from the ASF dual-hosted git repository. sjanc pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mynewt-newtmgr.git
commit ec0007b2a64ab77518c2fade28eefc1fb09207c2 Author: Szymon Janc <[email protected]> AuthorDate: Thu Oct 22 00:58:22 2026 +0200 nmxact/xact: Fix image upload window deadlock The image upload main loop blocked on an unbuffered channel send when the window was full, and a response handler received from it only if it observed the transition from a full window to one with a free slot. With sessions which deliver responses quickly (serial, naked BLE, or any session under load), a handler often saw the full window before the main loop got to check it. The handler then blocked as a stale receiver while the main loop found a free slot and went on. The next time the window was full, the stale receiver let the main loop send one request too many. With the window size growing by one per response while the count of outstanding requests shrank by one, no handler saw WCap == WCount anymore and the main loop waited forever: "newtmgr image upload" with a window size > 1 occasionally hung (and leaked a goroutine per stale receiver). Replace the handshake with a non-blocking notification on a channel with a buffer of one: each handler signals after processing a response or error and the main loop re-checks the window after every wake-up. A notification is never lost, as a pending one is kept in the buffer, and handlers no longer block. Add window sizes 2 and 5 to the upload tests and a stress test which reliably hung before this change. Assisted-by: Claude:claude-opus-5-5 --- nmxact/xact/image.go | 27 ++++++++++++++------------- nmxact/xact/image_upload_test.go | 30 +++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/nmxact/xact/image.go b/nmxact/xact/image.go index 3963c89..7de5745 100644 --- a/nmxact/xact/image.go +++ b/nmxact/xact/image.go @@ -329,7 +329,10 @@ func (t *ImageUploadIntTracker) HandleError(off int, err error) bool { func (c *ImageUploadCmd) Run(s sesn.Sesn) (Result, error) { res := newImageUploadResult() - ch := make(chan int) + + // Signalled (without blocking) whenever a response or error has been + // processed and a window slot may have been freed. + ch := make(chan int, 1) rspc := make(chan nmp.NmpRsp, c.MaxWinSz) errc := make(chan error, c.MaxWinSz) @@ -344,8 +347,8 @@ func (c *ImageUploadCmd) Run(s sesn.Sesn) (Result, error) { for int(atomic.LoadInt32(&t.MaxRxOff)) < len(c.Data) { // Block if window is full - if !t.CheckWindow() { - ch <- 1 + for !t.CheckWindow() { + <-ch } t.ProcessMissedChunks() @@ -378,17 +381,15 @@ func (c *ImageUploadCmd) Run(s sesn.Sesn) (Result, error) { go func(off int) { select { case err := <-errc: - sig := t.HandleError(off, err) - if sig { - <-ch - } - return + t.HandleError(off, err) case rsp := <-rspc: - sig := t.HandleResponse(c, rsp, res) - if sig { - <-ch - } - return + t.HandleResponse(c, rsp, res) + } + + // Wake up the main loop if it is waiting for a free slot. + select { + case ch <- 1: + default: } }(int(r.Off)) } diff --git a/nmxact/xact/image_upload_test.go b/nmxact/xact/image_upload_test.go index 1354cc6..f3c928c 100644 --- a/nmxact/xact/image_upload_test.go +++ b/nmxact/xact/image_upload_test.go @@ -131,6 +131,8 @@ func TestImageUploadRun(t *testing.T) { {"one chunk", oneChunk, 1}, {"one chunk + 1", oneChunk + 1, 1}, {"many win 1", 10000, 1}, + {"many win 2", 10000, 2}, + {"many win 5", 10000, 5}, } for _, tt := range tests { @@ -296,7 +298,7 @@ func TestImageUploadRunProgress(t *testing.T) { func TestImageUploadRunRetransmit(t *testing.T) { data := testImage(5000) - for _, winSz := range []int{1} { + for _, winSz := range []int{1, 5} { s := newFakeSesn(nil) s.mtu = 200 @@ -332,6 +334,32 @@ func TestImageUploadRunRetransmit(t *testing.T) { } } +// Window based uploads used to deadlock occasionally: a response +// handler could signal a free slot while the main loop was not waiting, +// leaving a stale signal behind which let the loop overfill the window. +// With the window then growing while the count shrank, no handler +// observed a full window anymore and the main loop waited forever. +// The deadlock depends on goroutine scheduling, so run many uploads. +func TestImageUploadRunWindowStress(t *testing.T) { + data := testImage(2000) + + for _, winSz := range []int{2, 5} { + for i := 0; i < 50; i++ { + dev := &fakeImgDev{} + s := newFakeSesn(dev.handle) + s.mtu = 128 + + c := NewImageUploadCmd() + c.Data = data + c.MaxWinSz = winSz + if _, err := runCmd(t, c, s); err != nil { + t.Fatalf("win %d iter %d: %v", winSz, i, err) + } + checkUploaded(t, dev, data) + } + } +} + func TestImageUploadRunMtuTooSmall(t *testing.T) { dev := &fakeImgDev{} s := newFakeSesn(dev.handle)
