On Jun 26, 2008, at 8:33 PM, bulia byak wrote:

On Thu, Jun 26, 2008 at 10:49 PM, MenTaLguY <mental@...3...> wrote:

...

As has been demonstrated in this and many other cases, using hard assertions during development is the best way to guarantee that bugs are found and fixed quickly.

These particular crashes are found and fixed, yes. But how can you be sure that there are no other assert-crashes that are missed by our sketchy developer-testing, get released, and make Inkscape feel crashy and flakey to the users - who therefore may feel disinclined to even report them?

This seems to be where we get into the subtle choices about assertions, warnings, exceptions, etc.

Generally an assertion should be a "this should never, ever, ever happen" thing in the code. For development and testing builds assertions can be "turned on", and then for release they can be "turned off"

Warnings should be for "I don't *think* this will happen, but if it does I really want to know about it. just in case"

Exceptions should be for "normally this bit of code will work, but when an unusual case hits we'll want to stop and recover"

and then return value error codes should probably be for "this call will not succeed in the normal course of things, so I need to check whether or not it did"

For Inkscape, I'm not familiar with the state of assertions and if they should be assert(), ASSERT(), g_assert(), etc. It's likely that we probably want to avoid bare assert() or g_assert() calls, and perhaps use some macro. Runaway memory, failure to malloc, etc. might fall into this case. (Most projects in C or C++ I've been on that have used asserts have wrapped them in turnon/turnoff macros)

If we have some code that gracefully handles some error condition and recovers from it, then that sounds like the time for a g_warning() call to be added at that point. Those allow operation to continue, but then developers can run with --g-fatal-warnings and get an exception thrown at the end of warning display.

For something low-level, throwing an exception might be the best there. So for our current issue the lib itself should optionally do a g_warning() (depending on precise code needs, etc) and then throw an exception. Up higher a calling routine (that happens to know more of the big picture, and more of what would be an appropriate recovery action) can catch any exception, log it, and then recover.

On Jun 26, 2008, at 11:24 PM, MenTaLguY wrote:

Without assertions, invariant failures still tend to eventually result in crashes, *especially* in C/C++. Removing assertions would certainly help to obscure the cause of a crash, but crashes would not be entirely prevented, only rendered less predictable (and therefore less likely to be caught by our sketchy developer-testing).

Shooting the messenger does not really fix the problem.

Yes. We need to allow the messengers in...

... then drop a two-ton anvil on whatever monster chased them out to begin with.

(Yes, I'm a graduate of the Chunk Jones school of coding)

If we start squishing the messengers, then they'll stop running to us, and the big daddy monsters will stop following them into our traps.

On Fri, 2008-06-27 at 12:41 +0200, J.B.C.Engelen@...1578... wrote:

This is why I have proposed to replace assertions with exceptions in lib2geom (and already did a few). Then we don't have to remove/disable the assertions, and we end up with a system that can recover on error.

My primary concern is whether or not assertion failures get hidden from the end user. Exceptions are adequate in principle for indicating invariant failures, but what usually happens is that people start writing things like:

try { // ... } catch (AnyLibraryException e) { return some_default_value; }

For "normal" exceptions, that may not always be an unreasonable approach, but exceptions resulting from the violation of an invariant should not be lumped in with "normal" exceptions.

At minimum, a failed invariant means that there is a significant bug which may result in undefined behavior, and not infrequently in C++, failure to observe an invariant is either the result or the cause of memory corruption.

For LPEs, exceptions instead of asserts have proven to be much easier to work with. Instead of getting a bug report: "when i wiggle this line somewhere special, it crashes, don't know when". I get: "for the attached SVG, I get an LPE error". (LPE 2geom exceptions are handled by simply aborting the calculation and showing the original path, so it is very easy to see when an error happens and because Inkscape doesn't crash, you can save the data and attach it to a bugreport)

The most important thing is that assertion failures are visible to the user. Crashing is a rather blunt instrument approach to doing this, so I am not opposed to more graceful ways in principle (particularly if they facilitate the collection of debugging information).

Do we have a flag in Inkscape for release/debug that can be used to #ifdef some code?

Let's please not start #ifdeffing things. The less difference there is between the release and debug codebases, the better.

For example, the parser calling code now catches the 2geom exception, outputs path string to screen and then rethrows the exception. For release only, instead of rethrowing it might want to return an empty path instead of throwing an exception.

This would be exactly the sort of sweeping under the carpet that I am concerned about. At minimum you should distinguish between simple parse errors (e.g. due to invalid input data), which *can* sometimes be reasonably treated relatively silently in this sort of way, from assertion failures, which should *always* be propagated up to the UI somehow.

If the invariant failure happens to be associated with memory corruption, you are not doing the user any favors if you hide the fact that Inkscape is now unstable and they are putting their work at risk by not saving and exiting right then.

-mental

On Fri, Jun 27, 2008 at 2:24 AM, MenTaLguY <mental@...3...> wrote:

Without assertions, invariant failures still tend to eventually result in crashes, *especially* in C/C++. Removing assertions would certainly help to obscure the cause of a crash, but crashes would not be entirely prevented, only rendered less predictable (and therefore less likely to be caught by our sketchy developer-testing).

With the assert-crashes, on the other hand, right now I am forced to go back to an old build in order to complete a design project, because these constant crashes are really driving me nuts. Now whose win is that?

You can print a megabyte of debug dump. You can beep. You can flash. You can draw a big green monster on my screen. Just please, PLEASE don't crash, OK? Is it too much to ask for?

On Fri, 2008-06-27 at 12:26 -0400, bulia byak wrote:

You can print a megabyte of debug dump. You can beep. You can flash. You can draw a big green monster on my screen. Just please, PLEASE don't crash, OK? Is it too much to ask for?

Johan and I talked about this at length last night; while there are some issues which need to be thought through and dealt with (e.g. not losing backtraces), popping up a dialog seems like a reasonable compromise for an invariant failure. However, as noted before I can't completely guarantee that Inkscape wouldn't still crash on its own (or worse, silently corrupt/lose data) in those cases. The ultimate problem is that there are real bugs that have to be fixed.

Something which may be of more immediate practical help to you is that we've identified some assertions which need not really be assertions, since some of the checks because of their context effectively become checks of input data rather than data structure integrity. With some work, those can be separated out and treated a little more "softly". If we're really lucky, those are predominantly the ones you are hitting right now.

-mental