fix: Job.Call deadlocks on same-goroutine re-entrancy - #4
Open
eanfs wants to merge 4 commits into
Open
Conversation
* 'main' of https://github.com/eanfs/gotask: 将队列长度从 128 增加到 256 Upgrade queue to 128 # Conflicts: # event_loop.go
Job.Call 在 Size>0 时无条件把 callback 入队事件循环、再 <-ctx.Done() 阻塞等 它执行。若调用方本身就是事件循环 goroutine(在某 task 的 Start()/Go() 或另一个 被 Call 的 callback 里又调 Call),入队的 callback 永远不会被处理(循环正阻塞在 这次 Call 里),导致永久死锁——下游如 util.Manager 的 SafeGet/SafeRange(L==nil 时走 Call)在发布钩子里被重入调用即触发。 - 新增纯 Go goID()(解析 runtime.Stack 首行,无汇编/cgo) - EventLoop 记录 run() 所在 goroutine 的 loopGID(运行期非0,退出置0) - Call 入队前用 onLoopGoroutine() 检测重入,命中则内联执行 callback (已在循环 goroutine 上,与排队处理语义等价,单 goroutine 串行) - 新增回归测试 Test_Call_ReentrantFromLoopGoroutine(无修复时3s超时死锁)
There was a problem hiding this comment.
Pull request overview
Fixes a deadlock in Job.Call when it is invoked re-entrantly from the same event-loop goroutine (e.g., from inside a task Start() or another Call callback) by detecting “already on loop goroutine” and executing the callback inline.
Changes:
- Add goroutine-ID detection (
goID()) and track the event loop goroutine ID whileEventLoop.run()is executing. - Update
Job.Callto run callbacks inline when called from the loop goroutine to avoid self-deadlock. - Add a regression test that reproduces the re-entrant deadlock scenario.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
job.go |
Adds re-entrancy detection to execute Call callbacks inline on the loop goroutine. |
event_loop.go |
Records/uses the event loop goroutine ID for Call re-entrancy detection. |
goid.go |
Introduces a pure-Go helper to extract the current goroutine ID from runtime.Stack. |
reentrant_call_test.go |
Adds regression coverage for Job.Call re-entrancy from the loop goroutine. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+119
to
+123
| e.loopGID.Store(goID()) | ||
| ch := e.getInput() | ||
| e.cases = []reflect.SelectCase{{Dir: reflect.SelectRecv, Chan: reflect.ValueOf(ch)}} | ||
| defer func() { | ||
| e.loopGID.Store(0) |
Comment on lines
+229
to
+233
| if mt.eventLoop.onLoopGoroutine() { | ||
| mt.Debug("call inline (reentrant)", "caller", caller) | ||
| callback() | ||
| return | ||
| } |
Comment on lines
+21
to
+25
| i := 0 | ||
| for i < len(b) && b[i] >= '0' && b[i] <= '9' { | ||
| i++ | ||
| } | ||
| id, err := strconv.ParseInt(string(b[:i]), 10, 64) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Job.Call(whenSize>0) unconditionally enqueues the callback onto the event loop and blocks on<-ctx.Done()waiting for it to run. If the caller is itself the event-loop goroutine (e.g. inside a task'sStart()/Go(), or inside anotherCalled callback that re-entersCall), the enqueued callback is never processed (the loop is blocked inside this veryCall) → permanent deadlock.This bites downstream consumers: e.g. a
util.Manager'sSafeGet/SafeRange(which route throughCallwhenL==nil) called re-entrantly from a publish/start hook deadlocks the whole manager event loop, hanging all subsequent operations on it.Fix
goID()(parsesruntime.Stackfirst line; no asm/cgo, cross-platform)EventLooprecords the goroutine id of itsrun()inloopGID(non-zero while running, 0 after exit)CallchecksonLoopGoroutine()before enqueuing; if the caller is already the loop goroutine, run the callback inline (semantically equivalent to the loop processing it — single-goroutine serialization, no deadlock)Test_Call_ReentrantFromLoopGoroutine(3s-timeout deadlock without the fix; passes with it)Notes
Test_Hooksnil-pointer,Test_ParentStoppost-completion assertion,lessons_CNtimeout) fail on the default branch independently of this change.github.com/langhuihui/gotask(no unrelated changes).