search

mutagen-io / mutagen

Fast file synchronization and network forwarding for remote development
https://mutagen.io
Other
3.47k stars 153 forks source link

Log unhandled errors if they occur #508

Open mikeschinkel opened 3 months ago

mikeschinkel commented 3 months ago

NOTE: This is a LARGE PR but it only does one thing. If you may plan to accept it, you should do before you add a lot of new code or it will just be too hard to resolve the conflicts. HOWEVER, this PR changes the API so to follow SemVer I think you would need to bump the major version number.

Purpose

This PR replaces finds almost all places where an error is returned by a function — but the Mutagen code just ignores the error — and it adds logging in case an error is returned.

I discovered these issues while I was making changes for my own needs on my fork for #505. My IDE kept complaining about unhandled errors, so I decided to fix them and move to a separate PR. As it turned out, fixing them was quite the rabbit hole, but I did so because I know having all unhandled errors logged will make for more robust software and potentially reveal edge-case bugs in the code you did not even realize where there.

Serendipitously DoltHub published a blog post entitled _"What's the best Static Analysis tool for Golang?" that starts with an example of an unhandled error as justification for why handling all errors is so important. I say "serendipitously"_ because it was published after I wrote the code for this PR.

What the PR Changed

  1. The PR adds a package named must which includes functions for most of the places where errors were being ignored, such as Close(), Shudown(), etc. Then is replaces the code that looks like this:

    item.Close()

    With code that looks like this: 

    must.Close(item,logger)

  2. In order to ensure a logger was always available, this PR adds: a. A logger property to every struct that has a method where unhandled errors occurred, or b. A logger parameter for every package function where unhandled errors occurred, and c. Ensured those properties were set and parameters were passed in all cases.

  3. In the very few cases that use interfaces I decided to handle the error manually so as not to modify the interface.

  4. In one case I could not find a reasonable way to get a logger so I just left a TODO comment.

Summary

I really hope you will consider accepting this. It was a few days of work to both find and fix all the case and to them get all the CI tests to pass. If you do, I think it will help you make Mutagen even more robust moving forward.