Skip to content

Update the ecs error handler to allow being updated more than once #18886

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

Closed
wants to merge 1 commit into from

Conversation

Brezak
Copy link
Contributor

@Brezak Brezak commented Apr 20, 2025

Objective

The application error handler can be set only once.

Solution

Use an atomic pointer to store the error handler pointer and allow setting it at any time.

Testing

Added 2 new tests.


Showcase

The Bevy ecs error handling system has been changed to allow updating the error handler.

fn error_handler(error: BevyError, context: ErrorContext) {
   // ... do stuff.
}

// before
GLOBAL_ERROR_HANDLER.set(error_handler);

// after
set_error_handler(error_handler);
// do it twice, why not.
set_error_handler(error_handler);

@Brezak Brezak force-pushed the atomic-error-handler branch 6 times, most recently from ab66679 to 5a4956a Compare April 21, 2025 00:14
@Brezak Brezak force-pushed the atomic-error-handler branch from 5a4956a to 326a496 Compare April 21, 2025 00:23
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Apr 23, 2025
@alice-i-cecile
Copy link
Member

How do you feel about this versus #18810? I would also like to see some relative benchmarks of these approaches: I very much want to just remove the feature flag.

@Brezak
Copy link
Contributor Author

Brezak commented Apr 23, 2025

How do you feel about this versus #18810? I would also like to see some relative benchmarks of these approaches: I very much want to just remove the feature flag.

I prefer their solution. Though I'd like to eventually make the error handler a Box<dyn Fn(...)>.

What would you like me to benchmark? Error handling is usually the cold path so I don't actually know where we'd see slowdowns.

@alice-i-cecile
Copy link
Member

The initial impl shows huge slowdowns on the command benchmarks :( That said, I also prefer the linked solution, so I'm going to close this out and concentrate our efforts there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants