Re: [Bug-wget] Disable assertions by default

[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.


--
Thanking You,
Darshit Shah

pgpI9V0ztPgex.pgp
Description: PGP signature