fix: Fix race condition #248
Conversation
When a process running in a separate go-routine, e.g., a fetch request is represented by a promise in JS, the result of the process is enqueued on the task queue of the main go routine. However, converting the result from a native Go value to a JavaScript value should have occurred in the main go-routine, not the concurrent task. This could cause concurrent calls to the same JavaScript context, which isn't allowed.
WalkthroughThis PR introduces a scoped clock execution model: Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| func (c *Clock) enter() { c.stack++ } | ||
| func (c *Clock) exit() error { | ||
| c.stack-- | ||
| if c.stack > 0 { | ||
| return nil | ||
| } | ||
| return c.tick() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a defensive check for unbalanced calls.
If exit() is ever called without a matching enter() (e.g., due to a future refactoring mistake), stack would become negative and tick() would still run. While the current code correctly pairs these via Do(), a defensive check could help catch bugs early during development.
🛡️ Optional: Add defensive assertion
func (c *Clock) enter() { c.stack++ }
func (c *Clock) exit() error {
c.stack--
+ if c.stack < 0 {
+ panic("gost-dom/clock: exit() called without matching enter()")
+ }
if c.stack > 0 {
return nil
}
return c.tick()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *Clock) enter() { c.stack++ } | |
| func (c *Clock) exit() error { | |
| c.stack-- | |
| if c.stack > 0 { | |
| return nil | |
| } | |
| return c.tick() | |
| } | |
| func (c *Clock) enter() { c.stack++ } | |
| func (c *Clock) exit() error { | |
| c.stack-- | |
| if c.stack < 0 { | |
| panic("gost-dom/clock: exit() called without matching enter()") | |
| } | |
| if c.stack > 0 { | |
| return nil | |
| } | |
| return c.tick() | |
| } |
| // Tick runs all tasks scheduled for immediate execution. This is synonymous | ||
| // with calling Advance(0). | ||
| func (c *Clock) Tick() error { return c.Advance(0) } | ||
| func (c *Clock) tick() error { return c.Advance(0) } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Update comment to reflect the method is now private.
The comment still describes Tick as if it were a public API entry point. Since it's now private (tick), consider updating the comment to reflect its internal nature, or removing it since callers should use Do() instead.
📝 Suggested comment update
-// Tick runs all tasks scheduled for immediate execution. This is synonymous
-// with calling Advance(0).
+// tick runs all tasks scheduled for immediate execution (equivalent to Advance(0)).
+// This is called internally by exit() when the outermost Do() completes.
func (c *Clock) tick() error { return c.Advance(0) }| return m.ctx.do(func() error { | ||
| m.ctx.logger().Debug("Evaluate module", "vm", m.ctx.vm) | ||
| p := m.record.Evaluate(m.ctx.vm) | ||
|
|
||
| if p.State() != sobek.PromiseStateFulfilled { | ||
| return p.Result().Export().(error) | ||
| } | ||
| return m.ctx.tick() | ||
| if p.State() != sobek.PromiseStateFulfilled { | ||
| res := p.Result().Export() | ||
| err, ok := res.(error) | ||
| if !ok { | ||
| err = fmt.Errorf("run module: %v", res) | ||
| } | ||
| return err | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the file and understand the repository structure
find . -name "sobek_module.go" -type f | head -20Repository: gost-dom/browser
Length of output: 99
🏁 Script executed:
#!/bin/bash
# List relevant files in the scripting/sobekengine directory
git ls-files | grep -E "(sobek|script)" | head -30Repository: gost-dom/browser
Length of output: 1678
🏁 Script executed:
#!/bin/bash
# Inspect the sobek_module.go file around lines 20-33
find . -name "sobek_module.go" -type f -exec cat -n {} \; | head -50Repository: gost-dom/browser
Length of output: 907
🏁 Script executed:
#!/bin/bash
# Find the scriptContext type and its do() method
rg -n "func.*\(.*scriptContext.*\)\s*do\(" scripting/sobekengine/ -A 10Repository: gost-dom/browser
Length of output: 859
🏁 Script executed:
#!/bin/bash
# Search for scriptContext definition
rg -n "type scriptContext" scripting/sobekengine/ -A 20Repository: gost-dom/browser
Length of output: 1530
🏁 Script executed:
#!/bin/bash
# Search for Clock.Do method or related event loop handling
rg -n "Clock|EventLoop|RunTicks|Drain" scripting/sobekengine/ -A 5 -B 2Repository: gost-dom/browser
Length of output: 1617
🏁 Script executed:
#!/bin/bash
# Look for all go files in sobekengine to understand the structure
git ls-files scripting/sobekengine/ | grep "\.go$"Repository: gost-dom/browser
Length of output: 668
🏁 Script executed:
#!/bin/bash
# Find the Clock type and its Do method implementation
find . -name "clock.go" -o -name "clock*.go" | grep -v test | xargs cat -n 2>/dev/null | head -100Repository: gost-dom/browser
Length of output: 4604
🏁 Script executed:
#!/bin/bash
# Search for Clock.Do method definition across the codebase
rg -n "func.*Clock.*Do\(|func \(c \*Clock\)" --type=go -A 15Repository: gost-dom/browser
Length of output: 15141
🏁 Script executed:
#!/bin/bash
# Look for the clock package
git ls-files | grep clock | grep -v test | grep "\.go$"Repository: gost-dom/browser
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Search for tests or usage patterns involving module evaluation
rg -n "sobekModule|Run\(\)|Evaluate.*module" --type=go -B 2 -A 3 | head -60Repository: gost-dom/browser
Length of output: 3260
🏁 Script executed:
#!/bin/bash
# Look for any handling of PromiseStatePending or promise settlement patterns
rg -n "PromiseState|PromisePending|moduleRecord\.Evaluate" --type=go -B 2 -A 2Repository: gost-dom/browser
Length of output: 404
🏁 Script executed:
#!/bin/bash
# Check if there are comments or tests explaining expected module behavior
rg -n "top.?level.*await|module.*settle|async.*module" --type=go -i | head -30Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check for tests related to module evaluation
find . -name "*sobek*test.go" -o -name "*module*test.go" | xargs grep -l "module\|Run\|Evaluate" 2>/dev/nullRepository: gost-dom/browser
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Look at the sobek_test.go file to see how modules are tested
cat -n scripting/sobekengine/sobek_test.go | head -100Repository: gost-dom/browser
Length of output: 1314
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about module expectations in sobek_module.go
cat -n scripting/sobekengine/sobek_module.goRepository: gost-dom/browser
Length of output: 907
🏁 Script executed:
#!/bin/bash
# Check if Sobek documentation or examples mention promise settlement
rg -n "Evaluate|Promise|module" scripting/sobekengine/ --type=go | grep -i "comment\|doc\|test" | head -20Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look at scripttests to understand what modules are being tested
find . -path "*/scripttests*" -name "*.go" | head -10Repository: gost-dom/browser
Length of output: 592
🏁 Script executed:
#!/bin/bash
# Check the module test suite to see what module behavior is expected
rg -n "RunModuleSuite|TestESM" scripting/internal/ --type=go -A 5Repository: gost-dom/browser
Length of output: 625
🏁 Script executed:
#!/bin/bash
# Search for any top-level await or async module handling in tests
rg -n "await|async|top.?level" scripting/internal/ --type=go | head -20Repository: gost-dom/browser
Length of output: 701
🏁 Script executed:
#!/bin/bash
# Get the full module test suite to see what scenarios are tested
cat -n scripting/internal/scripttests/modules_test_suite.go | head -200Repository: gost-dom/browser
Length of output: 6960
🏁 Script executed:
#!/bin/bash
# Check if sobek.ModuleRecord.Evaluate ever returns a pending promise, or if it always settles synchronously
rg -n "ModuleRecord\|\.Evaluate" scripting/sobekengine/ --type=go -B 2 -A 5Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for references to Sobek library or documentation about module evaluation
rg -n "github.com/grafana/sobek" . --type=go | grep -i "doc\|comment\|import" | head -10Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for any documentation or issues about async modules or top-level await
rg -n "top.*level|async.*module|awaiting.*module" . --type=go --type=md | head -20Repository: gost-dom/browser
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Check V8 module implementation for comparison
cat -n scripting/v8engine/module.go | head -50Repository: gost-dom/browser
Length of output: 1476
🏁 Script executed:
#!/bin/bash
# See the full awaitPromise implementation in V8
cat -n scripting/v8engine/module.goRepository: gost-dom/browser
Length of output: 2421
🏁 Script executed:
#!/bin/bash
# Check if there's a similar awaiting mechanism for Sobek
rg -n "awaitPromise|await.*Promise|promise.*settle" scripting/sobekengine/ --type=go -A 5Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look at how Sobek handles promises in general
rg -n "Promise|promise" scripting/sobekengine/ --type=go | head -30Repository: gost-dom/browser
Length of output: 1121
🏁 Script executed:
#!/bin/bash
# Check Sobek documentation/issues about module evaluation and promise states
rg -n "PromiseState|Fulfilled|Pending|Rejected" scripting/sobekengine/ --type=goRepository: gost-dom/browser
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Verify if there's any mechanism in Sobek to wait for promises like V8's PerformMicrotaskCheckpoint
rg -n "Microtask|checkpoint|drain" scripting/sobekengine/ --type=goRepository: gost-dom/browser
Length of output: 42
Pending promise incorrectly treated as error—module evaluation does not await promise settlement.
The promise state is checked synchronously within Clock.Do() before microtasks are drained. For modules with top-level await (which return a pending promise), the check at line 24 will treat PromiseStatePending as a failure.
Compare to the V8 implementation (scripting/v8engine/module.go), which explicitly awaits the promise via awaitPromise() to allow settlement before returning. The Sobek implementation lacks this promise-awaiting mechanism and will reject async modules.
Recommend adding explicit promise settlement logic similar to V8, or ensuring Clock.Do() drains microtasks before the state check occurs.
When a process running in a separate go-routine, e.g., a fetch request is represented by a promise in JS, the result of the process is enqueued on the task queue of the main go routine.
However, converting the result from a native Go value to a JavaScript value should have occurred in the main go-routine, not the concurrent task. This could cause concurrent calls to the same JavaScript context, which isn't allowed.