Friday Facts #296 - All kinds of bugs

Regular reports on Factorio development.

Koub
Global Moderator
Global Moderator
Posts: 7627
Joined: Fri May 30, 2014 8:54 am
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Koub »

These remind me so much about when I was in school, learning to code, and trying to debug stuff XD
Koub - Please consider English is not my native language.

User avatar
Optera
Smart Inserter
Smart Inserter
Posts: 2920
Joined: Sat Jun 11, 2016 6:41 am
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Optera »

That logic is "correct" in that it does what it was supposed to do: open any gates that the train could drive over. What it wasn't accounting for was a rail system where everything looped back onto itself 5-10 times per junction.The time complexity for the algorithm it was using was O(N^2). That's "fine" when N is small. However, in this save file, with this rail network, and with these modded trains (with 2,500% speed bonus from modded fuel no less) it meant N ended up somewhere around 75,865. That - as it turns out - was slow.
Finally a reason beside the terrible look to ban roundabout junctions from servers. ;)

User avatar
Philip017
Filter Inserter
Filter Inserter
Posts: 360
Joined: Thu Sep 01, 2016 11:21 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Philip017 »

Im sure glad you like to fix these bugs, because we sure dont like it when they break our game we are tring to enjoy

thank you for all your hard work, know it is appreciated! :D

Blacky007
Fast Inserter
Fast Inserter
Posts: 178
Joined: Fri Dec 29, 2017 8:05 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Blacky007 »

Image

the same junction with working all parallel drive through

Image

and yes finding those bugs is hard.
I have an open bug I'am unable to find it.
viewtopic.php?f=29&t=62155

Keep up the good work @DEVs
My color birthday was May 2nd 2020 - Thank you Enchroma

UPS_Eater
Manual Inserter
Manual Inserter
Posts: 3
Joined: Fri Jun 08, 2018 11:14 am
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by UPS_Eater »

As a budding programmer, this has been very interesting to read and I will keep this page as it has very good advise. Thank you for posting these!

svalorzen
Inserter
Inserter
Posts: 21
Joined: Fri Mar 18, 2016 11:44 am
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by svalorzen »

A thing I tend to do for these cases, when the code is supposed to be very performant, is just add a ton of asserts. Then the game crashes the same, but when you debug the problem you can get much closer to the source by starting to look from the first failed assert rather than the last thing which explodes due to the chain of impossibilities.

User avatar
Shingen
Long Handed Inserter
Long Handed Inserter
Posts: 92
Joined: Fri Jan 04, 2019 3:25 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Shingen »

blah blah missing signal in red circle blah blah OCD blah blah blue-circled signals do nothing blah blah
factorio_missing_signal.png
factorio_missing_signal.png (912.22 KiB) Viewed 14399 times
svalorzen wrote: ↑
Fri May 24, 2019 11:02 am
A thing I tend to do for these cases, when the code is supposed to be very performant, is just add a ton of asserts. Then the game crashes the same, but when you debug the problem you can get much closer to the source by starting to look from the first failed assert rather than the last thing which explodes due to the chain of impossibilities.
when code is supposed to be very performant i assume i've written it properly and don't add asserts which slow it down constantly, but instead when something does break, i just work my way backwards from the code that breaks. if a bug is reproducible it's really not a problem.
actually, i do that also when it's not supposed to be very performant.
because most likely the bug is gonna happen in such a way that they're not gonna help you anyway :v
Last edited by Shingen on Fri May 24, 2019 11:39 am, edited 2 times in total.

Dave64738
Inserter
Inserter
Posts: 29
Joined: Wed Apr 13, 2016 10:37 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Dave64738 »

Klonan: If you're interested in sorting out overflow errors maybe you could take a look at the Production Scrap bug where creating a boiler results in attempting to create 4.3G stone furnaces? If you try to cancel it this results in the map and your inventory being spammed with tons of stone. The author has "fixed" it by disallowing intermediate products, which suggests it could be problem in the base game rather than his mod - like this one I think something small is going negative and being converted to unsigned, and also because stone -> (high chance of boiler, small chance of just getting stone back). But it doesn't occur when you just try to create a stone furnace; it has to be an intermediate furnace.

I did have a savegame where this was 100% reproducible but I think I deleted it. I can have a go at recreating it if you like. The problem is reproducible in very early game: just get some stone and iron and try to create a boiler.

programaths
Burner Inserter
Burner Inserter
Posts: 6
Joined: Fri Feb 22, 2019 6:44 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by programaths »

The best thing to do when you encounter the NPE is work out why and if you should or not correct in place.

If not, then you should still catch and throw a new exception and set the original exception as a cause. This permits to add extra information that can help the calling code to discriminate this exception against a general NPE one.

Also, you can hint the developer by explaining what went wrong in the message and in the documentation of the exception.

Lastly, in Java (for java fellow), where you put a comment to document an assumption, remove it to add an explicit "assert" statement. These are a noop on normal circumstances, but can be enabled while testing. Also, they are more visible than comments AND clearly state what they are: an assertion.

In C or any languages with preprocessing, macros can be used.

Bilka
Factorio Staff
Factorio Staff
Posts: 3299
Joined: Sat Aug 13, 2016 9:20 am
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Bilka »

Dave64738 wrote: ↑
Fri May 24, 2019 11:39 am
If you're interested in sorting out overflow errors maybe you could take a look at the Production Scrap bug where creating a boiler results in attempting to create 4.3G stone furnaces?
That was fixed a while ago: 68836
I'm an admin over at https://wiki.factorio.com. Feel free to contact me if there's anything wrong (or right) with it.

User avatar
eradicator
Smart Inserter
Smart Inserter
Posts: 5206
Joined: Tue Jul 12, 2016 9:03 am
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by eradicator »

programaths wrote: ↑
Fri May 24, 2019 11:45 am
The best thing to do when you encounter the NPE is work out why and if you should or not correct in place.
Error: Acronym overflow has occured near "NPE".
Author of: Belt Planner, Hand Crank Generator, Screenshot Maker, /sudo and more.
Mod support languages: ζ—₯本θͺž, Deutsch, English
My code in the post above is dedicated to the public domain under CC0.

bNarFProfCrazy
Fast Inserter
Fast Inserter
Posts: 194
Joined: Sat Apr 23, 2016 7:11 am
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by bNarFProfCrazy »

I really like these in depth details.

xng
Fast Inserter
Fast Inserter
Posts: 165
Joined: Fri Feb 14, 2014 1:04 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by xng »

You're in good company with the signed integer cast problem, that is the reason Gandhi in Civilization is so aggressive (his aggressiveness was -1). The bug was introduced in the first game and because it was an interesting bug to have Gandhi as a nuke maniac he still is the same in every new iteration of the series.

User avatar
ledow
Fast Inserter
Fast Inserter
Posts: 102
Joined: Sat Sep 24, 2016 3:00 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by ledow »

NULL checks are critical and should be absolutely everywhere. And result in instant failure of the application, with full debugging trace.

The first line of any new C function that I write is literally just a list of any pointer parameters with:

assert(parameter != NULL);
assert(parameter2 != NULL);

That stops it "breaking" the program, but it doesn't stop it having NULL sent to it - which is what I think you're getting at. Catching the invalid values is the safety net to stop an unintended crash and instead crash *knowingly*. Actually stopping the crash happening is another matter entirely.

And, (I program in C99 so this is different and much easier in C++) any access to any other structures should be managed by a getter/setter function. Don't just blindly assume that if you're passed a large structure, you can just iterate through it, plug in values inside it and anything else you find in there (like other pointers to other structures), etc. Create it as a type, use getter/setter functions for that type, and deny access to the pointer to the underlying data as much as possible.

Then the getters/setters can validate and test functionality as they go (not to mention centralise and *log* accesses so debugging is easier), functions used come with a set of constraints/invariants/assumptions/whatever you want to call them regarding the validity of the data they are acting on using the official functions, and accidents like this result in asserts all over the shop (at least in any debug build) with trace logs from the very first instance.

Also, you need to *extra carefully* validate any data that comes from outside... this includes mods, saves, translation strings, image files read from disc, everything. You can't just act on the mod's code as if -1 was a valid value, from the very beginning.

It's hard, but after a while, you start to program like that as a force of habit. And if you use the right data-types, you aid yourself a lot. (Again, I'm sure there's a fancy C++ way to do this, but I tend to program with typedef's types for "unsafe_file" and "parsed_file" or similar, and there's literally only one function that takes or can act on an "unsafe_file" and that's the parser function that returns a "parsed_file" type).

If those functions mentioned had had asserts on negative numbers, if they'd had asserts on the parsing routines for negative numbers, if they'd had no way to turn a file with -1 into a valid "parsed" file, e.g. no dumb-casting of values into an unsigned value so it can only hold 0 or above and doesn't see -1 as 4 billion, and if all your access to such values - in-game and by mods - was by a single set of getters-setters that are constrained by these asserts, then you could have had a fighting chance at discovering it before someone was able to send code to a game on my computer that could overflow the memory and potentially access things it shouldn't be able to.

It's all about funnelling down *all* possible access - internal, debugging, modding, scripting, "cheating" (e.g. AI giving itself a thousand free fuel or whatever) to one getter/setter which then has one set of all possible checks on the validity of its values, and nobody else has any access to it. Every shortcut through that path might be "performance enhancing" but it's all "security bypassing".

Then when you disable asserts/debugging, the code optimises away to nothing (almost all modern compilers will inline short validation snippets, remove asserts in production, or even completely remove checks that test only assumptions that can never be true etc.), when you refactor code you have one place to change, and when you have a crash a simple debug run goes BANG, BANG, BANG, assert, error, debug, "here's a problem"...

I know I'm preaching to the converted, but so many times such simple things as "we just assume mods know what they are doing", "we special-case mods so they can do what they like", or even "we don't bother to validate our image textures because they came from us so they must be fine" (where a simple substitution of a texture with a malicious one then gives someone else user privilege next time the game is loaded, etc.) result in major problems that are only patched up after the event, where they should be in there *by design* of the structure of the code.

Rakshasa
Long Handed Inserter
Long Handed Inserter
Posts: 97
Joined: Sat May 31, 2014 7:21 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Rakshasa »

ledow wrote: ↑
Fri May 24, 2019 12:50 pm
NULL checks are critical and should be absolutely everywhere. And result in instant failure of the application, with full debugging trace.

The first line of any new C function that I write is literally just a list of any pointer parameters with:

assert(parameter != NULL);
assert(parameter2 != NULL);
The point of 'assert' is that you can compile it with NDEBUG defined to disable it when making a release build, thus allowing you to have as many asserts as you want while working on the code without affecting production code.

dasiro
Fast Inserter
Fast Inserter
Posts: 130
Joined: Fri Jun 03, 2016 5:55 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by dasiro »

everyone loves a good bug-hunt (once it's solved) :)

User avatar
Omnifarious
Filter Inserter
Filter Inserter
Posts: 272
Joined: Wed Jul 26, 2017 3:24 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Omnifarious »

Shingen wrote: ↑
Fri May 24, 2019 11:32 am
when code is supposed to be very performant i assume i've written it properly and don't add asserts which slow it down constantly, but instead when something does break, i just work my way backwards from the code that breaks. if a bug is reproducible it's really not a problem.
actually, i do that also when it's not supposed to be very performant.
because most likely the bug is gonna happen in such a way that they're not gonna help you anyway :v
I rarely trust myself to generate test cases that truly exercise all the code paths or combinations thereof, and so I write a test that generates significant swaths of the possible input space and tests all of them. Even then I've sometimes missed things.

User avatar
Dev-iL
Filter Inserter
Filter Inserter
Posts: 303
Joined: Thu Jul 02, 2015 2:48 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by Dev-iL »

Rseding wrote: ...why would this code ever say to make a negative amount of smoke?" So I kept going ... why?
  • ... Because the logic to make those signed integers was "(cycle progress ...) - (last cycle progress ...)" (cycle was < last cycle ... that should never be possible)
  • Because the furnace burner had made a negative amount of progress in burning the fuel (negative progress should not be possible)
  • Because the "remaining amount of fuel from this item to burn" was negative (negative fuel values are invalid and the game won't even reach the main menu if some mod tries to set one)
  • Because the mod API didn't prevent mods from doing: entity.burner.remaining_burning_fuel = -1 AND the game didn't properly clear "remaining amount of fuel from this item to burn" when the item being burnt was removed due to mod migration/removal.
Good job with the mental stack-trace ;)
Leading Hebrew translator of Factorio.

User avatar
dog80
Filter Inserter
Filter Inserter
Posts: 287
Joined: Thu Dec 08, 2016 11:57 pm
Contact:

Re: Friday Facts #296 - All kinds of bugs

Post by dog80 »

that fking smart part of the brain

Post Reply

Return to β€œNews”