Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent stackoverflow #23

Merged
merged 2 commits into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 22 additions & 22 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 8 additions & 2 deletions src/source/promises.bs
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,13 @@ namespace promises.internal
if storage = invalid then
' unregister any observers on the promise to prevent multiple callbacks
promises.internal.unobserveFieldScoped(promise, promises.internal.PromiseField.promiseState)
promises.internal.observeFieldScoped(promise, promises.internal.PromiseField.promiseState, promises.internal.notifyListeners)
promises.internal.observeFieldScoped(promise, promises.internal.PromiseField.promiseState, sub(event)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the important piece here. This observer fires synchronously, which would then trigger upstream promises to also trigger synchronously (since they were all already marked resolved).

By adding a nexttick, it allows each promise's observers to run nexttick, breaking the stack chain.

'run the notification nexttick to prevent stackoverflow due to cascading promises all resolving in sequence
promises.internal.delay(sub(context)
promises.internal.notifyListeners(context.event)
end sub, { event: event })
end sub)

storage = {
promise: promise
thenListeners: []
Expand Down Expand Up @@ -482,7 +488,7 @@ namespace promises.internal
print "Crash during utils.delay:", e
#end if
end try
m[delayId] = invalid
m.delete(delayId)
Copy link
Member Author

@TwitchBronBron TwitchBronBron Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the main issue. I noticed these timer entries were never getting properly cleaned up, as they were only being set to invalid. This should clean up the m variable nicely now.

end sub)

timer.control = "start"
Expand Down
33 changes: 32 additions & 1 deletion src/source/promises.spec.bs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ namespace tests
promises.onThen(promise, sub(_ as dynamic, ctx as dynamic)
elapsedTimeInMillis = ctx.timespan.totalMilliseconds()
? "elapsed time to resolve promise:" + elapsedTimeInMillis.tostr()
tolerance = ctx.timerDurationInMillis * 0.1
tolerance = ctx.timerDurationInMillis * 0.2
msg = "did not settle within 10% tolerance of timer duration"
m.testSuite.assertTrue(ctx.timerDurationInMillis - tolerance <= elapsedTimeInMillis, msg)
m.testSuite.assertTrue(ctx.timerDurationInMillis + tolerance >= elapsedTimeInMillis, msg)
Expand Down Expand Up @@ -352,6 +352,37 @@ namespace tests
end sub, testNode)
end function

@async(60000)
@it("unravels deep promise chain without crashing due to stackoverflow")
function _()
'this function creates a promise that depends on another promise (until we hit a max)
doWork = function(context)
'if we hit the max, resolve the promise and unravel the entire stack
if context.currentCount > 10000
return promises.resolve(true)
end if
context.currentCount = context.currentCount + 1

'return a promise that depends on another future promise
return promises.onThen(promises.resolve(true), function(result, context)
doWork = context.doWork
return doWork(context)
end function, context)
end function

promises.chain(promises.resolve(true), {
currentCount: 0,
doWork: doWork
}).then(function(result, context)
doWork = context.doWork
return doWork(context)
end function).then(function(result, context)
m.testSuite.done()
end function).catch(function(error, context)
print "error", error, FormatJson(error.backtrace)
end function).toPromise()

end function
end class
end namespace

Expand Down
Loading