Re: [Bug-wget] Disable assertions by default

[Tim Rühsen]

Am Samstag, 22. November 2014, 09:55:23 schrieb Darshit Shah:
> On 11/21, Tim Rühsen wrote:
> >> Let's get this patch through first and others to handle the old
> >> assertions
> >> can flow in over the next week.
> >
> >Yes, looks good to me. Go push it.
> >
> >More comments below.
> >
> >Tim
> >
> >On Friday 21 November 2014 15:46:36 Darshit Shah wrote:
> >> On 11/21, Tim Rühsen wrote:
> >> >On Friday 21 November 2014 13:19:18 Darshit Shah wrote:
> >> >> On 11/20, Ángel González wrote:
> >> >> >On 20/11/14 15:29, Darshit Shah wrote:
> >> >> >>--- a/src/progress.c
> >> >> >>+++ b/src/progress.c
> >> >> >>@@ -992,6 +992,7 @@ create_image (struct bar_progress *bp, double
> >> >> >>dl_total_time, bool done)>>
> >> >> >>
> >> >> >>      {
> >> >> >>      
> >> >> >>        int percentage = 100.0 * size / bp->total_length;
> >> >> >>        assert (percentage<= 100);
> >> >> >>
> >> >> >>+      assert (false);
> >> >> >>
> >> >> >>        if (percentage<  100)
> >> >> >>        
> >> >> >>          sprintf (p, "%3d%%", percentage);
> >> >> >>
> >> >> >>-- 2.1.3
> >> >> >
> >> >> >Surely you didn't want to include this :)
> >> >> 
> >> >> Shit! No, that was meant to be for my own debugging purposes. I was
> >> >> trying
> >> >> to see if I could induce an assertion failure. Good thing the patch
> >> >> goes
> >> >> through review first.
> >> >> 
> >> >> I've fixed this in the attached patch.
> >> >
> >> >Just a comment.
> >> >In case the assertions are disabled, there still should be a check and a
> >> >WARNING message. It should inform the user that something weird happened
> >> >and the mailing list should be informed. Wget should try to continue if
> >> >possible. We just had the report that an assertion occurred after hours
> >> >(and many GB) of downloading and Wget just stopped... It was just one
> >> >file
> >> >out of thousands that would have been skipped...
> >> 
> >> I agree. We should add more logging and a warning message for when a file
> >> cannot be downloaded. And for such recursive cases, the same information
> >> should be displayed at the end of the operation too. This is currently
> >> missing.
> >> 
> >> >IMHO. we should have something like this ASAP. And having this, we also
> >> >might get rid of assertions completely. That could make this patch
> >> >obsolete.
> >> 
> >> I think we should not get rid of assertions. Some of the assertions, like
> >> the one at init.c:819 or progress.c:1180 are not for handling run-time
> >> errors but for intimating future developers about a logical error
> >> immediately. Such checks need to remain in the development code, but is
> >> worthless in production code.
> >
> >How can you make sure that a developer runs into the assert at init.c:819 ?
> >I guess the only sure way to check the 'commands' array is by calling
> >test_commands_sorted(). Only a 'make check' does it.
> 
> I can't recollect how right now. But I remember triggering that assertion a
> large number of times when I was adding a new option to Wget to for the
> first time. While coming to grips with the code base, I'd often make a
> mistake somewhere in one of the three different places which you have to
> edit to add a new option. This assertion would always trigger and let me
> know that I'm doing something very wrong.
> 
> >> Which is why I believe that assertions should remain. Just not all the
> >> ones
> >> we currently have.
> >
> >Some assertions surely *can* remain. What I try to say is: we do not need
> >them. A well thought of message / action can always replace an assertion.
> 
> A well thought of message replaces all assertions that need to remain in
> production code. Some assertions are meant to make the code fail instantly
> when some logic doesn't add up. For example the assertion I added at
> progress.c:1180 should never be available on end-user systems. It makes
> Wget fail instantly in certain scenarios when it would otherwise just
> continue downloading. However, as a developer, I'm interested in knowing
> immediately when that assertion fails. Because it implies that the length
> of the progress bar being drawn exceeded the size of my screen. This means
> there's an error in the logic in create_image() which needs to be debugged.
> 
> Such are the scenarios where a developer wants an assertion, not a debug
> message. Because I *want* Wget to crash. But in a lot of cases in the Wget
> codebase, assertions have been used instead of error messages. I've been
> looking at those recently and over the next week will try to remove quite a
> few.

Yeah, that sounds good.

If a certain assertion is a real help (and thus really makes sense) for us, of 
course let's keep it.

Tim

signature.asc
Description: This is a digitally signed message part.