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

Backtraces on commands apply #846

Closed

Conversation

alec-deason
Copy link
Member

@alec-deason alec-deason commented Nov 12, 2020

I often run into a bad debugging experience where a Command fails during writing and it's very hard to track down which system actually caused the issue because the backtrace is all inside bevy_ecs. I'd like to have backtraces captured when the Command was actually written to the buffer.

This change adds a new feature flag called "command_backtraces" which causes a failed Command to print a trace of the stack when it was added to the buffer as well as a trace of the actual failure point. It is gated by a feature because it depends on std::backtrace which is a nightly only API and because recording the backtraces will have some performance impact (though I haven't actually measured how large it is).

The primary thing I am concerned about in this PR is the way I've introduced the Result return into Command implementations. I don't feel like I have good intuition about best practice error handling in Rust so I'd love feedback about clarity and idiomicity (which is surely a word?).

@alec-deason
Copy link
Member Author

One other potential concern with this change. I was worried that it would reduce the usefulness of the backtraces from the actual failure point inside the Command but because of how anyhow captures and displays backtraces I don't think that's actually an issue unless there's a case I'm not noticing.

@memoryruins memoryruins added the C-Feature A new feature, making something new possible label Nov 12, 2020
@joshuajbouw
Copy link
Member

Please rebase

@alec-deason
Copy link
Member Author

It's been a while since I did a rebase and I think that could have been done better.... But it should be consistent with master now.

I'm even less satisfied with the error handling after these changes though. I ended up doing a bunch of anyhow!("{:?}", e) type conversions and that feels wrong to me.

@bjorn3
Copy link
Contributor

bjorn3 commented Feb 6, 2021

and because recording the backtraces will have some performance impact (though I haven't actually measured how large it is).

The performance impact of stack unwinding is non-trivial. Even when it is only done for a backtrace and not to perform cleanup after a panic. DWARF based unwinding requires searching through .eh_frame for the current function, interpreting all unwind instructions and then repeating it again for the parent function and so on.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@alec-deason
Copy link
Member Author

@alice-i-cecile I don't think the ready for cart label is appropriate on this one. There has been some discussion on discord about the state of error handling in this and a suggestion to use track_caller to resolve some of the concerns. But I haven't followed up on those yet.

@alec-deason alec-deason marked this pull request as draft February 20, 2021 06:44
@DJMcNab DJMcNab removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 23, 2021
@NathanSWard
Copy link
Contributor

I believe this can be closed in favor of #2241

@alice-i-cecile
Copy link
Member

Closing in favor of #2241 <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants