Jump to content
Sign in to follow this  
Followers 0
  • entries
    3
  • comment
    1
  • views
    1224

About this blog

Tales of coding by @evandixon

Entries in this blog

 

Processes, Events, and Debugging

The Background I'm a professional developer using mainly C#. On the side, I write a ROM editor using VB.Net. They're very similar thanks to them mostly wrapping the .Net Framework, and because of how complex this application is, I'm in no hurry to rewrite it. This application knows the ins and outs of a few dozen file types and can, well, edit them. One such file type is a compiled LUA script. In order to edit these, they must first be decompiled. Rather than try to figure out that myself, I use a 3rd party tool that can do that for me (sometimes the best code is the code you didn't have to write). To use it, I point it to the compiled script on disk, and it spits out the decompiled text into its standard output. Integrating with it is simple thanks to .Net: I can use a Process object to start it, redirect its standard output, and read the decompiled text as it's written. This is all part of a larger process, where transparent to the user, I decompile a bunch of these scripts, automatically make some changes, and recompile them, all in one go, so people can play a game using different characters. A few days ago, I did some refactoring to it so I can more easily get notified of unsuccessful exit codes when recompiling (previously the program would just continue fine, and any files with syntax errors would simply remain unmodified). Unfortunately for me, I was in a hurry and didn't test anything except that program before I pushed the change to the master branch and let my CI server publish a development build, currently the only way for people to get precompiled versions of my application. (The CI server is Team City, since they've generously given me unlimited everything with my open source license.) Also I don't have any tests covering this part of the code. The Problem One of my application's users encountered some (well, dozens) of exceptions when my program tried to recompile the decompiled scripts. I try reproducing the issue, and I indeed find a syntax error in the same spot the newly-created error message said it was. After looking closer, I found the issue was there before the decompiled scripts were automatically modified. The script looked something like this: function addTheNumbers(numberA, numberB) function addTheNumbers(numberA, numberB) return numberA + numberB return numberA + numberB end end print(addTheNumbers(10, 20)) print(addTheNumbers(10, 20)) It should be fairly obvious what the problem here is. The lines are doubled. I facepalm on the inside and start investigating. The Adventure Because the code I changed is being called about a dozen times on different threads, I did the responsible thing and wrote a test for it. After getting my test to fail for the right reason, I look at my code, and here's what I see: That looks like pretty standard stuff. The variables "captureConsoleOutput" and "captureConsoleError" are both true, telling the Process to redirect standard output and standard error, and the code will proceed to register event handlers. When I place breakpoints on the event handlers, I find that they're being hit twice per line the LUA decompiler writes. I scour the code and make sure that I only call Start once, and the event handlers are only added once as shown. I tried a bunch of other things too. What you see above is actually after another thing I tried. Previously, I was dumping standard output and error into the same StringBuilder , in case the program was outputting to both for some reason. I finally went to GitHub to check what actually changed when I did the thing mentioned in The Background above, and while it was a substantial change to the class shown above, it all seems pretty sane. But then I spot it. One crucial thing that didn't change. Oh. The event handler was registered twice. I added lines 39 and 40 (the two AddHandler lines) and forgot completely about the Handles clause. How Events Work And Why It Took Me So Long To Spot This Events are magical things that can be called, and event handlers that have been registered to the event are run, being given the data that was provided when the event was called. This isn't so much like method calls as it is interrupts. The syntax for adding an event handler in C# looks like this: myEvent += myEventHandlerMethod; And the syntax for adding an event handler in VB.Net looks like this: AddHandler myEvent, AddressOf myEventHandlerMethod Usually C# and VB.Net are very similar thanks to the .Net Framework, but sometimes there are some differences, where one language can do a thing that the other cannot. There are some gotchas when it comes to events, so they need to be used with care. .Net is a framework that tries its best to clean up after you and prevent memory leaks. Simply remove all references to an object, and .Net makes it go away. But when you register an event handler to an event in another class, that event handler has a reference to the event, and by extension the class. If you get rid of the references to that class, the reference to the event is still there, and .Net can't clean it up. But you also can no longer remove the event handler since the reference to the class is gone. That's why it's important to remove event handlers as you would pointers in C/C++. The syntax for removing an event in C# looks like this: myEvent -= myEventHandlerMethod; And the syntax for removing an event in VB.Net looks like this: RemoveHandler myEvent, AddressOf myEventHandlerMethod To help make managing this easier, VB.Net has some extra syntax. Instead of manually adding event handlers, you can declare a variable as WithEvents, then use the Handles clause on a method, like so: Private WithEvents _myMemberVariable As SomeRandomClassWithEvents Private Sub _myMemberVariable_OnPropertyChanged(sender as Object, e as PropertyChangedEventArgs) Handles _myMemberVariable.PropertyChanged 'Do the thing End Sub With no additional setup, when I assign something to _myMemberVariable, _myMemberVariable_OnPropertyChanged will automatically be run whenever _myMemberVariable.PropertyChanged is raised. If I change _myMemberVariable to some other SomeRandomClassWithEvents, it'll take care of removing the old handlers and adding the new ones for me. And setting _myMemberVariable to Nothing (aka null) will clean it up for me. I've been using C# a lot more lately, I've had to start doing events the old fashioned, no hand-holding way. I've been doing it for long enough now that I forgot to even check for the Handles clause when all evidence pointed to an event handler being registered twice. VB.Net is a fine language, and IMO it's easier to read and easier to type (not having to constantly type '{' and ':' when simply pressing enter works instead). However, it no longer has language parity with C#, meaning C# will continue to evolve, and VB.Net won't grow with it. The future is C#, and it's sad to see VB.Net be left behind. Guess I'll have to get around to rewriting this thing for it to be part of the future.
evandixon

evandixon

0
 

Adventures in C#: The dragon strikes back

At the company at which I work, we have this legacy application written in C#. (It's different from the one written in Classic ASP.) Let's call this the dragon. It's old, it's previously defeated many brave knights, and its name is an acronym that shares a letter or two with the word "dragon". The dragon is responsible for breathing fire to defend the village from intruders. (It uses coding and algorithms to do Things to Products because of business logic. While the details aren't important, I doubt I could say what it does beyond giving a brief overview of what it looks like to an outside observer.) However, the dragon also has this habit of attacking the knights who control it, so we need a new village gaurdian (the dragon is composed mostly of spaghetti code, and if we are ever to replace it, we need to understand how the spaghetti works). And so, a week or two ago I undertook a challenging task: to shrink the dragon so it'll fear the knights again (to refactor things to make it more readable). Armed with a mouse, keyboard, and the support of a fellow knight (a coworker who's dealt with this application before), I got to work so I could deal a mighty blow to the dragon. (Disclaimer: this post contains a lot of technical C# stuff that can't all be hidden behind dragon metaphores. Hopefully anyone who's not a programmer can still follow.) On my journey of code cleanup, the dragon presented many obstacles. A lot of it was superfluous spacing, redundant comments, and generally painful code, like so: //// //// Configure processing /// this.ConfigureProcessing(); //// //// Do the thing //// try { this.DoTheThing(); } catch { // eat it } if (this.SomeProperty == SomeValue) { // throw new SomeKindOfException(); // this was commented out // to-do: figure out why this was commented out } To make it even better, this.ConfigureProcessing does nothing and is referenced nowhere else, meaning we can easily remove most of the above. Also you should never ever ever ever ever catch an exception without handling it somehow. It will cause pain further down the line (but not in this blog post). If nothing else, log it somehow. Taking all of that into consideration, I reduced that code to this: try { this.DoTheThing(); } catch (Exception ex) { Trace.WriteLine($"Got an exception in TheType.TheFunction: {ex.ToString()}"); } There were also some things outside of general cleanup that I did. Imagine a class like this: public abstract class Magic : IAlgorithm { private string spellName = "Alakazam"; public string SpellName { get { return spellName; } set { if (value == null) { throw new ArgumentException("SpellName can't be null"); } spellName = value; } } private IWizard wizard = null; public IWizard Wizard { get { return wizard; } private set { if (value == null) { throw new ArgumentException("Wizard can't be null"); } wizard = value; } } #region IAlgorithm Methods public void SetWizard(string wizard) { this.Wizard = wizard; } public void Cast() { if (this.Wizard == null) { throw new Exception("Cannot call Execute before SetWizard"); } this.PerformMagic(); } #endregion protected abstract void PerformMagic(); } Not shown: 20 or more classes that inherit from this My fellow adventurer made the observation that the only times SetWizard was called was right after constructors of child classes, kinda like this: public Magic GetMagicInstance(string magicType, string spell, IWizard wizard) { Magic magic = null; switch (magicType) { case "white": magic = new WhiteMagic() { Spell = spell }; magic.SetWizard(wizard); case "black": magic = new BlackMagic() { Spell = spell }; magic.SetWizard(wizard); case "lost": LostMagic magic = new LostMagic() { Spell = spell }; magic.SetWizard(wizard); } return magic; } // Somewhere else calls magic.Cast after all instances of Magic have been created So using this knowledge and the knowledge of new C# features, we made the appropriate changes everywhere to make the class (and related classes) look like this: public abstract class Magic : IAlgorithm { public Magic(string spellName, string wizard) { SpellName = SpellName ?? "Alakazam"; Wizard = wizard ?? throw new ArgumentNullException(nameof(wizard)); } public string SpellName { get; } public IWizard Wizard { get; } #region IAlgorithm Methods public abstract void Cast(); #endregion } They say that if it bleeds, you can kill it. The dragon was certainly bleeding, if the code review tool was anything to go by: there were red lines (removals) all over the place in the pull request (Git terminology basically meaning we're publishing our changes). Two other knights in our party came to inspect our triumph (they reviewed the code to make sure it was done right). When it was all over, a total of 4 people (2 of us were involved in the refactoring) thoroughly reviewed everything and approved. Then we released the changes to be applied to the dragon down in the village (along with some other minor quality-of-life changes). About a week later*, one of the villagers called for help because the dragon that's supposed to defend the village was unable to breathe as much fire (a user opened a support ticket because the Things being done to Products weren't being done correctly). I was skeptical that any of my changes would have that effect. After all, when I shrunk the dragon, I made sure to quintuple-check my changes before releasing it to make sure its fire breath was as unchanged. So I investigated for a few hours to find out why. I ruled out places that could have been affected and was thoroughly stumped. That's when my fellow party member used his influence with the king to learn the secrets known only to the villagers (he used his higher permissions to debug the application, reading data from the production database). That's when I spotted the issue. Remember that constructor in the Magic class that was created? Take another look at the SpellName assignment: SpellName = SpellName ?? "Alakazam"; That should be: SpellName = spellName ?? "Alakazam"; In C#, variables are case-sensitive, so the property was being assigned to itself, and all Magic classes (WhiteMagic, BlackMagic, LostMagic, etc.) all only used the Alakazam spell. No wonder the flames were weak! We fixed the issue, and gave the villagers a dragon capable of properly defending them, and spent the rest of the day identifying how many intruders snuck into the village while the dragon was powerless to stop them (we had to find out which Products didn't have the Thing done properly). Conclusion Case sensitivity can potentially lead to a lot of pain and suffering. In this case with the constructor, that's standard practice in C#, and it's perfectly understandable that no one caught that one mistake, especially since it's just one letter in commit full of thousands of other changes. The tooling didn't catch it, because it checks for useless assignments. This wasn't useless, it says "make SpellName be Alakazam if it's null". Always be careful of casing when variable names are similar. * I massively simplified a lot of things. While a week is probably a bit long to go without noticing anything wrong, there's reasons. Also it didn't affect everything, because it just broke some hacks that are there because business logic.
evandixon

evandixon

0
 

Adventures in Classic ASP: Barcodes

By night and by weekend I'm an admin here at Project Pokémon (when I'm not busy saving Hyrule or doing other stuff). But by day, I'm a software developer for a certain company. This company has an internal legacy system written in Classic ASP (a really old language used to make websites) that we have to maintain, and releasing changes to it is never fun. Today we released a month's worth of changes, far more than we like to release at one time. (We couldn't release because reasons; although, we probably should have added a feature switch. Lesson learned.) Within this legacy system, there's a page that is designed to be printed out. Users will then scan an I2of5 barcode on it with a handheld scanner, making it easier to continue their work. After the updated the system, however, we got reports that the barcodes wouldn't scan anymore. Barcodes that were printed before this update worked fine. We reprinted the barcode that was working, and sure enough, the new release gave a different barcode generated from the same source text. I had my hands all over the page with this barcode, but I know I didn't touch the barcode itself. Thanks to source control, we were able to use git blame and looked at the entire code path responsible for the generation of this I2of5 barcode. This kind of barcode is generated by encoding some text into what looks like garbage, but looks like and scans like a barcode when this garbage is displayed using a particular font. Unfortunately, nothing in that code path has changed for years, and since we released this thing a month ago without any (related) issues, we had to find out what changed. Yet comparing the output of the old and new code showed different barcodes. The new barcode's garbage-looking text had some special characters in it called "replacement characters", which is a special kind of character to indicate something's wrong with the character encoding (more on that in a bit). We inspected the HTTP headers and HTML metadata, and there was no difference, so it wasn't a matter of the web browser interpreting the raw text data incorrectly. So we had to try something else. We used a git bisect (or rather a manual version of it, because of a bunch of branching weirdness - don't ask) and eventually found the exact commit that introduced the issue. The only thing that changed about the page in question is that a new ASP file was included. This ASP file, along with the ones it in turn includes, are simply containers for functions and classes and are not intended to write anything to the page. To debug what was causing the problem we removed parts of this included file until finally there was nothing left. We tried removing the reference to this file altogether, and that made the problem go away. We put it back and made sure the file was empty, and to double check, we removed that file and re-created it to make double sure it was empty. Turns out including an empty text file causes issues with our barcode! What is a text file? It's just a file containing bytes that programs interpret as text. But there's different kinds of text. ASCII text interprets one byte as one character. One byte can have a value from 0-255, and while standard ASCII only has characters for 0-127, extended ASCII has characters for 128-255 too. There's also variants of Unicode, where multiple bytes can be used for a single character, which is useful for characters that aren't found in English. How is a program to know what kind of text is in a text file though? By adding extra bytes to the beginning of the file to indicate the text encoding: the Byte Order Mark (BOM). Using HxD, I found that these "empty" text files weren't in fact empty, they had the BOM bytes to mark the file as UTF-8, which is a variant of Unicode that looks like ASCII until the 128-255 byte value range. By including this "empty" file, we were including the BOM, which changed the character encoding of the entire page, completely changing the meaning of the text that's used to display the barcodes (because it uses extended ASCII characters for some reason). Thankfully, this whole release didn't go as bad as it could have. Because of our Blue/Green deployment setup, we weren't under as much pressure as we could have otherwise been, and we only had to revert to the previous version twice, and the release only took 8 hours (because of testing and debugging other issues*). * Bonus material: when using Powershell to create a virtual application in IIS, note that "userName" is case sensitive in some versions of IIS. In 7.5, it's fine to use "username", but when using 8.5, that case sensitivity will cause setting the application credentials to silently fail. (Not that we encountered a situation exactly like this one or anything.)
evandixon

evandixon

1
Sign in to follow this  
Followers 0
×