- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8
fix: Ensure PlatformScheduleAction order is preserved #176
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
base: main
Are you sure you want to change the base?
Conversation
In Android/iOS these action are run on the main thread whereas in netstandard they are run in a threadpool and are not guaranteed to be run in order when we don't wait for the result.
| try | ||
| { | ||
| // Wait for the task to complete to ensure that actions are executed in the correct order. | ||
| Task.Run(a).Wait(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a timeout here? Or do we already not timeout, and require the application code return in a timely manner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think any amount of wait violates the AsyncScheduler contract, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, this probably isn't good on the caller side of the equation. It still does feel like we need a thread specifically to wait for these tasks, or a pool of 1 that we just submit to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a queue of tasks that get linked together as added and the next is dispatched as the previous finishes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that works as well, potentially very similar to the async queues we've implemented for the CSI spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated it to be a chained list of calls. The continueWith should still work even if an action throws an exception. This should allow us to keep the same contract but enforce the order.
| Bug: Non-Atomic Task Scheduling Causes Concurrency IssuesThe read-modify-write operation on  | 
| Task.Run(a); | ||
| // Chain the new action to run after the previous one completes | ||
| // This ensures actions run in order while maintaining fire-and-forget behavior | ||
| _lastScheduledTask = _lastScheduledTask.ContinueWith(_ => a()); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Race Condition in Task Scheduler
There's a race condition in PlatformScheduleAction. Concurrent calls can cause _lastScheduledTask to be updated non-atomically, which breaks the intended sequential ordering of scheduled tasks. This means tasks may execute out of order or in parallel, as volatile doesn't ensure atomicity for read-modify-write operations.
In Android/iOS these action are run on the main thread whereas in netstandard they are run in a thread pool and are not guaranteed to be run in order when we don't wait for the result.
Note
Ensure scheduled actions run in order on netstandard by chaining them via a shared Task instead of using Task.Run.
pkgs/sdk/client/src/PlatformSpecific/AsyncScheduler.netstandard.cs:private static volatile Task _lastScheduledTask = Task.CompletedTask.Task.Run(a)with chaining:_lastScheduledTask = _lastScheduledTask.ContinueWith(_ => a());to enforce ordered, fire-and-forget execution.Written by Cursor Bugbot for commit e0169d4. This will update automatically on new commits. Configure here.