take into account ctx.Done() when running timers - #7
Conversation
|
@kuoruan FYI |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces context support to the timers package, enabling proper cancellation of timer goroutines by passing a context.Context through InjectTo and NewTimers. While the implementation correctly refactors the timer loop to handle context cancellation, the review highlights critical concurrency concerns. Specifically, the Items map and NextItemID counter lack synchronization, and there is a data race on the Done field within the timer items. The feedback suggests implementing a sync.Mutex and using atomic operations to ensure thread safety. Additionally, several comments point out that passing the context as an argument to internal methods is redundant as it is already a member of the timers struct.
| type timers struct { | ||
| Items map[int32]*internal.Item | ||
| NextItemID int32 | ||
| ctx context.Context |
There was a problem hiding this comment.
The Items map and NextItemID counter are accessed concurrently by the V8 execution thread and timer goroutines (via ClearCB). Go maps are not thread-safe for concurrent read/write operations. You should add a sync.Mutex to the timers struct and use it to protect all accesses to these fields.
ctx context.Context
mu sync.Mutex| if t.Done { | ||
| return | ||
| } |
There was a problem hiding this comment.
There is a data race on the t.Done field. It is read here in the timer goroutine but can be modified concurrently by the V8 thread via the Clear() method (e.g., when clearTimeout is called) or by the goroutine itself at line 78. Consider using sync/atomic or a mutex to synchronize access to the Done and Cleared fields.
| "context" | ||
| "errors" |
There was a problem hiding this comment.
Add sync to the imports to support the addition of a mutex for thread-safe map access.
| "context" | |
| "errors" | |
| "context" | |
| "errors" | |
| "sync" |
| ctx := info.Context() | ||
|
|
||
| id, err := t.startNewTimer(info.This(), info.Args(), false) | ||
| id, err := t.startNewTimer(t.ctx, info.This(), info.Args(), false) |
There was a problem hiding this comment.
The ctx argument is redundant here because it is already stored in the timers struct as t.ctx.
| id, err := t.startNewTimer(t.ctx, info.This(), info.Args(), false) | |
| id, err := t.startNewTimer(info.This(), info.Args(), false) |
| ctx := info.Context() | ||
|
|
||
| id, err := t.startNewTimer(info.This(), info.Args(), true) | ||
| id, err := t.startNewTimer(t.ctx, info.This(), info.Args(), true) |
There was a problem hiding this comment.
The ctx argument is redundant here because it is already stored in the timers struct as t.ctx.
| id, err := t.startNewTimer(t.ctx, info.This(), info.Args(), true) | |
| id, err := t.startNewTimer(info.This(), info.Args(), true) |
| } | ||
|
|
||
| func (t *timers) startNewTimer(this v8go.Valuer, args []*v8go.Value, interval bool) (int32, error) { | ||
| func (t *timers) startNewTimer(ctx context.Context, this v8go.Valuer, args []*v8go.Value, interval bool) (int32, error) { |
There was a problem hiding this comment.
Remove the redundant ctx parameter and use t.ctx directly within the method.
| func (t *timers) startNewTimer(ctx context.Context, this v8go.Valuer, args []*v8go.Value, interval bool) (int32, error) { | |
| func (t *timers) startNewTimer(this v8go.Valuer, args []*v8go.Value, interval bool) (int32, error) { |
| t.Items[item.ID] = item | ||
|
|
||
| item.Start() | ||
| item.Start(ctx) |
There was a problem hiding this comment.
Use t.ctx directly since the ctx parameter is being removed from the method signature.
| item.Start(ctx) | |
| item.Start(t.ctx) |
There was a problem hiding this comment.
Pull request overview
This PR updates the timers polyfill to stop running timer goroutines once an associated Go context.Context is done, preventing timers from firing after the embedding environment has been torn down.
Changes:
- Add
context.Contextplumbing throughtimers.InjectTo,NewTimers, andItem.Startso timers can stop onctx.Done(). - Update timer execution loop to
selectonctx.Done()in addition to ticker events. - Store the provided context on the
timersinstance and pass it into newly created timer items.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
timers/timers.go |
Threads a Go context through timer creation and start logic. |
timers/internal/item.go |
Updates the timer loop to exit on ctx.Done() and changes Start signature. |
timers/inject.go |
Changes injection API to accept a Go context and constructs timers with it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| func InjectTo(iso *v8go.Isolate, global *v8go.ObjectTemplate) error { | ||
| t := NewTimers() | ||
| func InjectTo(ctx context.Context, iso *v8go.Isolate, global *v8go.ObjectTemplate) error { |
|
|
||
| func InjectTo(iso *v8go.Isolate, global *v8go.ObjectTemplate) error { | ||
| t := NewTimers() | ||
| func InjectTo(ctx context.Context, iso *v8go.Isolate, global *v8go.ObjectTemplate) error { |
| func NewTimers(ctx context.Context) Timers { | ||
| return &timers{ | ||
| Items: make(map[int32]*internal.Item), | ||
| NextItemID: initNextItemID, | ||
| ctx: ctx, | ||
| } |
| t.NextItemID++ | ||
| t.Items[item.ID] = item | ||
|
|
||
| item.Start() | ||
| item.Start(ctx) | ||
|
|
| func (t *Item) Start(ctx context.Context) { | ||
| go func() { | ||
| defer t.Clear() // self clear | ||
|
|
||
| ticker := time.NewTicker(time.Duration(t.Delay) * time.Millisecond) | ||
| defer ticker.Stop() | ||
|
|
||
| for range ticker.C { | ||
| if t.Done { | ||
| break | ||
| } | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return |
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| if t.Done { |
When running timers I noticed that when the context gets disposed, the timers are not cancelled, causing nil pointer issues because the timer tries to execute inside a context that does not exist anymore.
This makes sure that the timers get cancelled when the context is marked as done.
Breaking changes
This PR changes the signature of the timers
InjectTofunction, which breaks current implementations of this function.More information
I looking into using the v8go context, but this does not provide the correct information we need to get this to work