Discussion: More robust error handling #732

Activity

1. peq

   added

   unclear/discussion

   on Oct 1, 2018
2. ElusiveMori commented on Oct 2, 2018

   

   ElusiveMori

   on Oct 2, 2018

   Last edited by ElusiveMori

   More actions

   I saw you mention implementing some kind of try-catch system in Wurst as well, so I'm going to throw my 2 cents in here.

   We already have facilities in StdLib for (very rudimentary) catching of errors. The try() function from the Execute package will catch a crashing thread, and the last error (as a string) can be inspected using lastErrorMessage from ErrorHandling.

   I use this extensively in my own libraries, especially for FileIO stuff where a lot of things can go wrong (especially user-provided implementations for Persistable), and it works fine for catching errors, and carrying on in case of a failure. In fact, I rely on these errors being thrown to find out whether there was some kind of error in user code. If the thread doesn't crash, then I have no way of telling.

   This system can be extended to allow throw()-ing an Exception object instead of just setting a lastErrorMessage string, which could be later extracted by a corresponding try() function.

   I think you could use this approach on the language level to implement a try/catch mechanic. For better error handling, Wurst could include a default, 'hidden' try/catch block at all "Entry points", such as Trigger Actions, Conditions, Timer Callbacks, etc. with something like a default handler printing out Unhandled exception xxx...

   Then, the current built-in errors could be modified to throw Exception-s rather than simple strings, allowing the users to gracefuly handle them.

   I don't think it's a good idea to choose a "continue after error" approach, because it IMO leads to even more confusion. This was one of the biggest problems with trying to debug JASS code - if a native returned a null or something else that you didn't expect, then your code would do all kinds of wonky, unexpected stuff later on.

   On the other hand, I can totally get behind how this behaviour could be desired in other situations, where you just want to print an error or a warning and carry on anyway, for the sake of players. Some errors aren't that critical, after all. But it depends from case to case.

   I'd also like to comment on individual suggestions:

   Return the default value from method m in cases 1 and 2.

   I'm not sure that's a good idea. If some class method returns an instance of another class, this could easily lead to a cascade of null-pointers, obscuring the original root cause. In complicated hierarchies, this is almost guaranteed to happen, and with or without "continue after error", the code will not work correctly.

   Just continue after double frees (case 3)

   I think this is possible. Double-free is not a critical error, most of the times, and is sometimes just programmer oversight trying to accidentally delete the same object twice in the same function. If it is a critical error, it will have likely lead to NPEs before the Double-free, so if the thread crashes on NPE but not on Double-free, that's fine, I think.

   Destroy a random object or the oldest object in case 4 and use that space for the new object.

   This could lead to all kinds of weird, obscure corruption bugs on leaks. Out of memory errors usually indicate a massive leak, and is something that almost certainly must be fixed. On the other hand, it heavily depends on the user code. If someone has reached the 30k-something limit on objects, it might very well be the case that the "older" objects are out of scope anyway.

   "Index out of Bounds" (for multi-arrays)

   I think this is fine, personally. A lot of JASS natives return default values on incorrect input anyway, so it wouldn't be too different from that. So long as the error is correctly logged, it should be fine.
3. PhoenixZeng commented on Oct 8, 2018

   

   PhoenixZeng

   on Oct 8, 2018

   More actions

   for case 4
   30k-something limit is Frustrating
   Especially when I use the 1.27 version of the wc3 ( it is usually in china). i only can use 8191 object .
   I think the compiler can use more than one array at compile time to retain the index of these objects. and the number of arrays is configurable
   eg:

   limit = 8191 (or 32767 base on the wc3 version )
   let w = index div limit // sign to use which array
   let i = index mod limit // sign to the array-index
   

   This does not cause too much runtime burden.
4. Frotty commented on Oct 8, 2018

   

   Frotty

   on Oct 8, 2018

   Member

   More actions

   I'm not gonna repeat everything from other packages, but my main reason for this ticket is, that non-destructive errors, like double frees, cause the thread to crash, which often isn't desirable.
   It's an easy fix, and good to know error, but if it happens in a full game and breaks the map, it's not worth it.
   Errors that will most definitely cause issues should still stop the thread.
   Imo this could also be a part of wurst.build/runargs config.
5. wurstscript

   locked and limited conversation to collaborators on Aug 10, 2021
6. peq

   closed this as completedon Aug 10, 2021