-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Fix Plugin::build
detection
#8103
Conversation
crates/bevy_app/src/app.rs
Outdated
@@ -765,9 +765,9 @@ impl App { | |||
plugin_name: plugin.name().to_string(), | |||
})?; | |||
} | |||
self.is_building_plugin = true; | |||
self.building_plugin_counter += 1; |
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.
It's unlikely, but these should always be checked adds and subtractions. If this overflows (highly unlikely, though still incorrect), it can result in missing the check.
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 have to disagree, it's not highly unlikely, it's impossible. usize
is large enough to store every memory address and for each recursion depth, we need to at least keep track of Box<dyn Plugin>
. We will run out of memory addresses much sooner than we will reach usize::MAX
.
crates/bevy_app/src/app.rs
Outdated
@@ -765,9 +765,9 @@ impl App { | |||
plugin_name: plugin.name().to_string(), | |||
})?; | |||
} | |||
self.is_building_plugin = true; | |||
self.building_plugin_counter += 1; |
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.
Probably not something we need to fix in this PR, but this leaves App
in an incorrect state if the plugin build panics. We may want to use bevy_utils::CallOnDrop
here.
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.
OnDrop
does not work here because we pass self
to Plugin::build
. I've used catch_unwind
and unwind_resume
instead. I am not sure if using AssertUnwindSafe
here is incorrect, but I think this shouldn't be an issue because we are resuming the unwind before we can observe any broken invariants.
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 would prefer to handle this through the type system, but this PR fixes the issue
Objective
App::run
insidePlugin::build
fails if that plugin adds another plugin before callingrun
, because we don't keep track of recursive build calls.Solution
cant_call_app_run_from_plugin_build
test to add another plugin before callingrun
.