fix: command manager connect go routine leak - #869
Conversation
| // TestConnectContextCancelledGoroutineLeak demonstrates the goroutine leak in | ||
| // Connect. executionCh is unbuffered: if Connect exits via r.Context().Done() | ||
| // after waitExecution has already found a non-nil execution, the goroutine | ||
| // blocks forever trying to send on executionCh with no receiver. | ||
| // | ||
| // The test is probabilistic. | ||
| func (s *managerSuite) TestConnectContextCancelledGoroutineLeak(c *C) { | ||
| st := state.New(nil) | ||
| runner := state.NewTaskRunner(st) | ||
| mgr := cmdstate.NewManager(runner) | ||
|
|
||
| st.Lock() | ||
| task := st.NewTask("exec", "test cmd") | ||
| chg := st.NewChange("exec", "test change") | ||
| chg.AddTask(task) | ||
| st.Unlock() | ||
|
|
||
| // Pre-register the execution so waitExecution returns immediately | ||
| // with a non-nil value, before stopWait can be closed. | ||
| mgr.AddTestExecution(task.ID()) | ||
|
|
||
| // Pre-cancel the context so Connect can exit via r.Context().Done() | ||
| // while the goroutine is still trying to send on executionCh. | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() | ||
|
|
||
| r := httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) | ||
| _ = mgr.Connect(r, httptest.NewRecorder(), task, "stdio") | ||
| } |
There was a problem hiding this comment.
Regression test for the go routine leak.
| executionCh := make(chan *execution) | ||
| executionCh := make(chan *execution, 1) | ||
| go func() { | ||
| e := m.waitExecution(task.ID(), stopWait) | ||
| if e != nil { | ||
| // executionCh is buffered to avoid a goroutine leak when this is | ||
| // blocked indefinitely. | ||
| executionCh <- e | ||
| } | ||
| }() |
There was a problem hiding this comment.
The fix for the go routine leak.
benhoyt
left a comment
benhoyt
left a comment
There was a problem hiding this comment.
Thanks for this. Per voice discussion, let's make a couple of changes here:
- Let's split the PR into just the gouroutine leak fix (and associated test), and all the other tests. Then we can evaluate the latter tests independently.
- I'd still like to avoid the additional
goleakthird party dependency if at all possible. I found https://go.dev/doc/go1.26#goroutineleak-profiles, which is introduced in Go 1.26 as an experimental feature. Can we try that?
|
Looks like you might be able to do this with the recent stdlib |
|
Thanks for this, much better, and neat to be able to use synctest. Can we make this specific test a top-level test (rather than a check/suite one) so we only use synctest.Run in the one test, not around the whole package? (For when we add more tests to this package later.) Additionally, I've found I can only repro this one in every 5000 times ( |
Not without moving to
See #878 on how I think we can deal with this. This is how I weeded out a large portion |
benhoyt
left a comment
benhoyt
left a comment
There was a problem hiding this comment.
Okay, sounds good. Let's push ahead with this and try the stress test thing -- that's a good idea in any case.
2 participants
This is a port of a downstream project's fix to the command manager
connection handling logic. Previous to this fix, the connect method could
leak a go routine if the
executionwas found, but the connect requestwas cancelled. The fix is to make the channel, which is sent on without a
select, a buffered channel of one.
This change also adds a regression test for the go routine leak.
Here is the test failure demonstrating the go routine leak.