support RELEASE_ASSERT and DEBUG_ASSERT for release and debug mode

Bug #1393821 reported by RJ Skerry-Ryan
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mixxx
Fix Released
Low
RJ Skerry-Ryan

Bug Description

From the discussion in https://github.com/mixxxdj/mixxx/pull/390:

Add a RELEASE_ASSERT macro:
* fatal (quits immediately) always
* prints a message to the log about the failure

Add a DEBUG_ASSERT macro:
* compiled away in release builds
* in debug builds, print a failure warning
* if DEBUG_ASSERT_FATAL defined, quit immediately

Add a scons flag, debug_asserts_fatal, default to 0.
* Define DEBUG_ASSERT_FATAL

RJ Skerry-Ryan (rryan)
Changed in mixxx:
status: New → Confirmed
assignee: nobody → RJ Ryan (rryan)
importance: Undecided → Low
milestone: none → 1.12.0
Revision history for this message
Sean M. Pappalardo (pegasus-renegadetech) wrote :

I wasn't following the discussion in the PR so I'll chime in here:
- We never want to quit immediately in a release build! We want to recover as much as possible.
- Doesn't ErrorHandler already address the RELEASE_ASSERT case, in that you can call it with a critical error and it will log it and attempt a graceful shutdown? (It just needs to be macro-ized.)
- Qt docs say that Q_ASSERTs are intended for development and are meant to be disabled in release builds. We should really add error-handling code after all asserts as if they aren't there so we can recover from problems as gracefully as possible for each one.

Revision history for this message
Sean M. Pappalardo (pegasus-renegadetech) wrote :

Also, why worry about people using development builds from the build server live? Why can't we just produce nightly release builds too for those people?

Revision history for this message
Owen Williams (ywwg) wrote :

I think the idea behind RELEASE_ASSERT is that it is only used if the program is about to crash anyway (in an unrecoverable fashion), so we might as well say why.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Sean -- please read the PR.

RELEASE_ASSERT would be extremely restricted in use (similar to our current use of Q_ASSERT) where it's only for cases where it's absolutely impossible to gracefully recover.

Rest assured, I will not let gratuitous RELEASE_ASSERTS in :).

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

> Also, why worry about people using development builds from the build server live? Why can't we just produce nightly release builds too for those people?

Some distros build Mixxx in debug mode. Some users follow our compilation instructions without thinking through the implications. I'm strongly for opt-in so people are aware of the risk.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

> Qt docs say that Q_ASSERTs are intended for development and are meant to be disabled in release builds. We should really add error-handling code after all asserts as if they aren't there so we can recover from problems as gracefully as possible for each one.

RELEASE_ASSERT is only for the situation where you really can't recover. We should work to remove those situations but the fact is that they exist currently and we just Q_ASSERT currently.

Revision history for this message
Sean M. Pappalardo (pegasus-renegadetech) wrote :

(Of course I read the PR before commenting here.)

This just sounds like reinventing the wheel. How is adding these macros any different from having proper error handling code and disabling asserts in release builds?

As for distros that package debug builds and blind-compilation users, we should change our default build flags to be release ones.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :
Changed in mixxx:
status: Confirmed → Fix Committed
Revision history for this message
Sean M. Pappalardo (pegasus-renegadetech) wrote :

Okaaaay... this just feels like it's going to come back to bite us. It's additional complexity at the very least.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

You're fencing with a straw man.

I got rid of 54 Q_ASSERTs. Not a single one of them had graceful error handling. If we compiled with Q_ASSERTs off Mixxx would segfault right after. Now every single one of them has graceful error handling and will not crash a release build.

RJ Skerry-Ryan (rryan)
Changed in mixxx:
status: Fix Committed → Fix Released
Revision history for this message
Swiftb0y (swiftb0y) wrote :

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/7661

lock status: Metadata changes locked and limited to project staff
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.