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

Provide assertion facilities on both release and debug mode #4352

Closed
fuzhe1989 opened this issue Mar 18, 2022 · 5 comments · Fixed by #4398
Closed

Provide assertion facilities on both release and debug mode #4352

fuzhe1989 opened this issue Mar 18, 2022 · 5 comments · Fixed by #4398
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. type/enhancement The issue or PR belongs to an enhancement.

Comments

@fuzhe1989
Copy link
Contributor

fuzhe1989 commented Mar 18, 2022

Enhancement

For some logic errors just throw is not safe: the process state might be already wrong. assert can be only used on debug mode. We might need a new way to assert some conditions on both release and debug mode.

It's output could be redirected to fatal.log.

@fuzhe1989 fuzhe1989 added type/enhancement The issue or PR belongs to an enhancement. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Mar 18, 2022
@fuzhe1989
Copy link
Contributor Author

This article introduces an implementation of an assertion macro.

@JaySon-Huang
Copy link
Contributor

JaySon-Huang commented Mar 22, 2022

What about just output to the stderr when we meet fatal error?

  • Now deployed by TiUP/TiDB-Operator, there is already something help us will collecting the output of stderr
  • I think "creating a file on disk" for outputting fatal message is not a very reliable way

@JaySon-Huang
Copy link
Contributor

JaySon-Huang commented Mar 22, 2022

I remeber that when TiDB panic/out-of-memory, it also use stderr for the panic log (I am not sure whether this behavior is limited by the Golang or not)

@fuzhe1989
Copy link
Contributor Author

@JaySon-Huang stderr is ok if:

  1. It won't lose any log record.
  2. It uses the same format as other logs.
  3. It could be correctly rotated.

Consider different envs (e.g. deployed by ti.sh), I'm not sure if stderr could handle all of those requirements well.

@breezewish
Copy link
Member

Looks like RUNTIME_CHECK and RUNTIME_ASSERT is not used. Maybe we should migrate current if (unlikely(...)) { .. } into this pattern?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants