goexif should not panic but return an error instead

[fawick]

goexif throws a panic for the attached image. Despite the GPS tags are malformed, the package should recover itself and simply return an error.

Quote from https://code.google.com/p/go-wiki/wiki/PanicAndRecover#Usage_in_a_Package: "By convention, no explicit panic() should be allowed to cross a package boundary. Indicating error conditions to callers should be done by returning error value."

Quote from http://blog.golang.org/defer-panic-and-recover: "The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values."

The relevant panic log:

panic: Tag format is not 'rational'

goroutine 495 [running]:
runtime.panic(0x9c5460, 0x1f132958)
        c:/go/src/pkg/runtime/panic.c:279 +0xe9
camlistore.org/third_party/github.com/camlistore/goexif/tiff.(*Tag).Rat2(0x1fa9e9b0, 0x0, 0x0,     0x743a66, 0x9c0de0, 0x1fa17440)
        c:/Users/dsde012/go/src/camlistore.org/tmp/build-gopath-  nosqlite/src/camlistore.org/third_party/github.com/camlistore/goexif/tiff/tag.go:238 +0x6f
camlistore.org/third_party/github.com/camlistore/goexif/exif.tagDegrees(0x1fa9e9b0, 0xb6c538, 0xe)
        c:/Users/dsde012/go/src/camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/third_party/github.com/camlistore/goexif/exif/exif.go:232 +0x3e
camlistore.org/third_party/github.com/camlistore/goexif/exif.(*Exif).LatLong(0x1f132930, 0x0, 0x0, 0x0, 0x0, 0x0)
        c:/Users/dsde012/go/src/camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/third_party/github.com/camlistore/goexif/exif/exif.go:254 +0x147

[fawick]

While tagDegrees and LatLong are functions added by camlistore, the issue remains valid. tiff/tag.go contains many panics that should be handled within the package.

[alimoeeny]

:+1:

[rwcarlsen]

Sounds good to me - I'll not defend my earlier Go code (not that my current skills are much better :). This should be a relatively straight forward change. PR's are welcome - otherwise I'll probably not get around to it until late this week or sometime next week.

[fawick]

Personally, I won't be able to come up with a PR for this issue any time soon as my TODO list is long enough already.

[rwcarlsen]

The TypeCategory panics (i.e. that occur in retrieval methods like mytag.Float(0)) can all be avoided by the user by checking the tag's TypeCategory prior to retrieving the value. This case seems a bit similar to an index out of bounds case - for which panics have been deemed appropriate. If you don't care about the small percent of time when an image might have corrupt TIFF IFD data, then you can just assume that certain tags have certain types - like assuming that some []byte always has length greater than zero (or some other length). And if the []byte might sometimes have unexpected lengths that you want to handle, then you first inspect the length and branch accordingly. The check is the same amount of code either way:

if tag.TypeCategory() != tiff.FloatVal {
    // handle error
}
f := tag.Float(0)

vs

f, err := tag.Float(0)
if err != nil {
    // handle error
}

I weakly lean toward the first style (currently implemented). But I could be persuaded otherwise.

The other panics in the tiff package should definitely be eliminated.

[fawick]

In my understanding, the official canon is that the Go language runtime itself indeed throws panics for out-of-bound situations, but that such out-of-bounds situation should be handled in code by recovering from it and using the error type to inform the calling code that something went astray. To further quote https://code.google.com/p/go-wiki/wiki/PanicAndRecover:

"Within a package, however, especially if there are deeply nested calls to non-exported functions, it can be useful (and improve readability) to use panic to indicate error conditions which should be translated into error for the calling function."

IMO this is where Go shows good usage of the robustness principle: Be prepared to encounter all kinds of erroneous situations from your input but protect your users from it as much as you can in your output. In case of a library package the users are fellow developers.

Applying the second style is better API design, too: From looking at the method interfaces any library user can see that he/she might receive an error, so he/she prepares handling the case that this error is != nil. Instead, there is no way the API itself tells the package user that he/she should brace his calling code for an eventual panic. I am aware that the documenation for Rat2 states that it might panic and I am convinced that every package user wants to read that and pay heed to it. Still, with an err as a second return value, the compiler enforces the calling code to react to it by complaining if you don't assign it. They might still decide to ignore it by assigning it to _, but at least they are required to become active about whether to anticipate and handle any wrong-goings before the code even compiles. In contrast, no compiler reminds a developer to handle a panic. Failing to pay heed to the documentation is a mistake which will get back at you only at runtime when the called package drags your program to digital hell and the panic callstack trace unwinds right before your eyes. A good library facilitates what the language has to offer to prevent things like that from happening.

So this all boils down to me politely disagreeing with your inclination and strongly advocating the second style.

[rwcarlsen]

See #24. I'd be interested in any other API breaking suggestions you might have.

[fawick]

Think of it as an API fix, not as an API break... ;-) No, honestly, I really do appreciate what you did here! Thank you very much!

[fawick]

Hmm, I better wait until #24 is merged into upstream before I close the issue.

[fawick]

Totally missed to close this when #24 got merged.