BuildStream doesn't behave correctly with assertions disabled
- Dominant language
- Python
- Stars
- 140
- Forks
- 45
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 6
Description
[See original issue on GitLab](https://gitlab.com/BuildStream/buildstream/-/issues/971)
In GitLab by [[Gitlab user @tristanvb]](https://gitlab.com/tristanvb) on Mar 25, 2019, 10:51
BuildStream was written with the assumption that assert statements are always enabled, but now we know it can be common practice in some production environments to disable assertions in python code.
The result is that running with assertions disabled is not really that safe.
Currently we have two failure modes:
* **A fatal error occurred**
This can mean we had a system error, unable to allocate some resource, or it can mean that the user has provided invalid input, for instance a malformed YAML file or such.
In all of these cases, we raise an error which derives from `BstError`, and the user gets an end user targetted error message explaining the problem and hopefully providing some recourse for the user to fix it.
If these kind of errors are untrapped, they can bubble up as stack traces, but that is a bug and really should never happen.
* **A programming error occurred**
This can mean that we end up in some state that we are unsure how we ended up there, but we know it is invalid, for instance when trying to checkout an artifact and the cache key doesnt exist.
It can also mean that there is a bug in a plugin somewhere, in which case we always want to present the user with a full stack trace; as this is more helpful for solving bugs. For the end user, this will show up as a `BUG` message in the UI.
These are all currently implemented with `assert` statements.
Since python programmers typically have a different expectation about assertions, such that they can be disabled at runtime and that the assertions are only used for debugging, it could make sense to create a new `Bug()` exception which derives directly from `Exception`, and replace our current `assert` statements with `raise Bug(...)` statements.
This might allow us to use assertions more vigorously, for instance to assert the validity of function arguments at every function entry point, and allow disabling the assertions at runtime, this would conform more to pythonic expectations.
Contributor guide
Research direction
Start with the original GitLab issue and inventory the Python assert statements involved in fatal errors and programming errors. Review how BstError failures and BUG stack traces are surfaced, then define the Bug exception behavior and scope before changing assertions. Done means disabled assertions no longer remove these safety checks while user errors and programming errors retain their intended handling.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- build-system
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100