-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix UB, panic and compiler error in bevy_tasks
#20352
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
Conversation
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.
Lgtm
Looks like |
I'm surprised this was not caught by CI. Are we somehow skipping web builds? |
There's nothing that would enforce this ATM. I don't think we have tests to enforce that the wasm |
I circulated this at work have have gotten some internal feedback. Working on some fixes, and marking this as |
Pushed a change that should fix some of the missing trait bounds on wasm tasks ( |
@NthTensor what are your plans here for the 0.17 release? |
I need to make this panic safe, @hymm has pointed out an place where UB can be caused by causing a panic in a scope and then later catching it. It's partly done, should be ready this evening. After that, I think we should be good to merge. |
I've addressed the feedback from the latest reviews. Most notably:
Seems like we are pretty close to being merge ready. |
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.
LGTM, though there's two things worth trying to see if we could improve the performance/readability here.
Alright, last round of feedback addressed. As follow up to this, I want to investigate integrating I think this should be good for merge now, but I'm going to go ahead and re-request reviews. |
# Objective Fixes several issues with `bevy_tasks` uncovered by @IceSentry and @Ratysz: + For web builds, `Task::is_finished` is missing, causing compile failures for cross-platform apps. + Scopes running on the single-threaded executor have possible UB due to mistakenly using a `'env` bound instead of a `'scope` lifetime bound on the scope ref. + Scopes running on the single-threaded executor fail to poll tasks spawned on them to completion when all tasks go to sleep at the same time (causing panics and potential UB). ## Solution - Replace `futures_channel::oneshot` with `async_channel::bounded(1)`, and use `Receiver::is_empty` to implement `Task::finished`. The `async_channel` channel crate was chosen because it allows `Task` to be `Send + Sync + Unpin` and because it has a special-case oneshot implementation for bounded size-one channels (inherited from `concurrent-queue`). - Switch around the incorrect lifetime bounds. - Use referencing counting to spin while ticking the executor until all tasks spawned on a scope are complete. Since these are all fairly small changes I rolled them together into a single PR. I can split any of the three out if they prove controversial. ## Testing - Changes are currently untested. I need to check that this fixes the bugs we've been seeing at work, and I'd like to run this through miri. It's concerning to me that miri failed to catch the bad lifetime bound on the scope impl.
Objective
Fixes several issues with
bevy_tasks
uncovered by @IceSentry and @Ratysz:Task::is_finished
is missing, causing compile failures for cross-platform apps.'env
bound instead of a'scope
lifetime bound on the scope ref.Solution
futures_channel::oneshot
withasync_channel::bounded(1)
, and useReceiver::is_empty
to implementTask::finished
. Theasync_channel
channel crate was chosen because it allowsTask
to beSend + Sync + Unpin
and because it has a special-case oneshot implementation for bounded size-one channels (inherited fromconcurrent-queue
).Since these are all fairly small changes I rolled them together into a single PR. I can split any of the three out if they prove controversial.
Testing
It's concerning to me that miri failed to catch the bad lifetime bound on the scope impl.