goblinfactory / konsole

Home of the simple console library consisting of ProgressBar, Window, Form, Draw & MockConsole (C# console progress bar with support for single or multithreaded progress updates) Window is a 100%-ish console compatible window, supporting all normal console writing to a windowed section of the screen, supporting scrolling and clipping of console output.
718 stars 62 forks source link

Seemingly impossible divide by zero exception thrown #89

Open PromoFaux opened 2 years ago

PromoFaux commented 2 years ago

Hi there,

This may have just been transient - and I've as yet been unable to reproduce it, but a user sent a screenshot of an unhandled exception caused by a divide by zero error:

image

But looking at your code for Konsole.ProgressBarSlim.Refresh(...., that seems impossible, as the potential for max being set to 0 is handled...

https://github.com/goblinfactory/konsole/blob/master/src/Konsole/ProgressBarSlim.cs#L103

For completeness, my code that is calling Refresh(current, itemText):

ProgressBar bar = new(cnt > halfSites ? right : left, 100, 53); // Window is split into two banks of prog bars (up to 80 on screen)
bars.Add(bar);
var updateDisplay = true;
bar.Refresh(0, site.SiteCode);
var iProgress = new Progress<ProgressTracker>(mp =>
{
    if (bar.Max != mp.NumFiles)
    {
        bar.Max = mp.NumFiles;
    }

    var val = mp.CurrentFile;
    var str = $"{site.SiteCode,-2} - {mp.TrafficDate:dd/MM} {mp.CurrentFile:0000}/{mp.NumFiles:0000} {mp.ImportStatusString}";

    if (updateDisplay)
    {
        bar.Refresh(val, str);
    }
}); //<----- This is Line 124

Anyway, this is more of a curiosity at the moment, feel free to close - Just figured I would flag it in case I am missing something incredibly obvious..! I have considered changing if (updateDisplay) to if (updateDispaly && bar.Max !=0), but that seems unnessacery

goblinfactory commented 2 years ago

Hi @PromoFaux

You have unfortunately caught me in middle of migrating my development machine and I wont be able to investigate this for a bit. I spent some time looking at the code and at first I thought it had something to do with a float never equalling zero, where I might need to change some code to write an extension method along the lines of .IsCloseToZero(...tolerance) but looking at the code I can't see that being the case.

I'm wondering if it's something odd with floats and implicit casting.

I agree with your title, very interesting.

My code combined with your error kind of suggests the following;

if max is zero, then return 0, but if max is some other non zero number that when cast to a decimal ...will result in zero? so, at least 1 question is, what non zero integer number when cast to a decimal could result in zero? hence your title.

Please leave this open; the error message suggests a guard clause somewhere, possible implicit casting error etc. Maybe there's something else we've missed so would prefer to leave this open.

I'm sure I can make that particular bit of code safer at the very least. these progressbars take an absolute hammering, so it's worth making this code bulletproof.

please ping me again in a few days if I've not pushed a hotfix for this.

txs for raising the issue, Cheers, Alan

goblinfactory commented 2 years ago

I'm labelling this a bug, so that it will get some eyeballs, and will remove with the next patch/fix. (I'm sure we can fix this even if this hardly ever occurs.)

PromoFaux commented 2 years ago

but if max is some other non zero number that when cast to a decimal ...will result in zero?

_max is an int, though, so it will only ever be a whole number, right? Even when cast as (decimal), it should be {_max }.0... maybe, so at minimum 1.0

And no worries on timing, like I said, I'm yet to be able to reproduce it so it's clearly an edge case.

goblinfactory commented 2 years ago

floats are sneaky bastards... I blame the floats somewhere .. just havent seen exactly how they did it! (grin) I'll take a look in detail in a few days, it's a really interesting bug. ciao, A