Use of panic in source code

[MarkSonghurst]

Hi I noticed the use of the Go panic() function within the source code, but no recover() functions. Are the higher level Go database/sql wrappers capturing the panics and recovering, or do I need to support recover() within my daemon when using database/sql with this odbc package?

Thanks for a great package. Mark.

[MarkSonghurst]

I cannot see any recover() functions within the Go database/sql files (I pulled 1.7 master). If I raise a PR which replaces the use of panic in your code with the returning of an error type is that ok, or are you using panic for a specific reason?

The Go convention https://github.com/golang/go/wiki/PanicAndRecover appears not to allow packages to panic outside of their own codebase, so it would be good for this package to remain compliant with that idiom.

[alexbrainman]

Are the higher level Go database/sql wrappers capturing the panics and recovering,

I don't think so.

do I need to support recover() within my daemon when using database/sql with this odbc package?

You are not expected to catch panics in odbc package. I expect your program will crash and print stack trace, and you will report the problem, and we will try to understand what happened.

If I raise a PR which replaces the use of panic in your code with the returning of an error type is that ok, or are you using panic for a specific reason?

I looked at all current panics in the code. They all safeguard against "programmer errors" (safety checks). If I know I made mistake in the code, should I continue?

Also the package is designed to talk to any ODBC compliant driver. But, personally, I have used just a few drivers. I wouldn't know how different drivers behave in corner cases. I don't want my code to continue, if ODBC driver behaves in an expected (or unknown to me) way.

Feel free to try and convert panics into errors, but I am not convinced it will improve the situation. What will your program do, if some odbc package function returns one of those "I don't understand how I end-up in this situation" errors?

Did you actually see any of these panics in real life? I didn't. And I don't remember them reported by others.

Alex

[MarkSonghurst]

Thanks for responding.

Feel free to try and convert panics into errors, but I am not convinced it will improve the situation. What will your program do, if some odbc package function returns one of those "I don't understand how I end-up in this situation" errors?

My daemon will handle the returned error gracefully and appropriately - raise an alert, log the issue and gracefully return an error message to the request originator. What it won't do is crash and dump stack, taking down the the daemon with it and dropping potentially hundreds of other simultaneous requests that are being processed.

Did you actually see any of these panics in real life? I didn't. And I don't remember them reported by others.

In real-life I cannot afford to allow panics to take down my daemon, which has paying customers relying on it's stability. Sure, decent testing should raise and make the causes of the panics visible at build time, but real-life is never predictable.

I have a PR almost done, I am just adding test cases.

[MarkSonghurst]

@alexbrainman PR #77 is available for your consideration. Thanks in advance for your time.

[bwmarrin]

So, I was just researching odbc drivers for Go for an upcoming project and stumbled upon this issue. I want to back up @MarkSonghurst here. I feel the use of an unrecovered panic within a package is an absolute no-no and is very non-idiomatic Go. I think Mark's approach here and reasoning is very sound. I echo his concern that I absolutely cannot risk having a package panic and crash an entire program that may be servicing hundreds of clients. Package errors should travel back up into the calling functions so the main program can handle them properly.

I vote (and ask) that Marks PR be cleaned up to contain only changes related to panic's specifically so it can be considered and hopefully merged.

Also, Mark I noticed you had some other fixes in your branch/PR that may be of benefit here but those should probably be submitted as independent PRs

For reference,

https://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."

https://github.com/golang/go/wiki/PanicAndRecover - "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. 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. "

[alexbrainman]

I vote (and ask) that Marks PR be cleaned up to contain only changes related to panic's specifically so it can be considered and hopefully merged.

I am always opened to reasonable changes. Small changes are preferable. But I won't be fixing panics myself.

Alex

[MarkSonghurst]

@bwmarrin feel free to use my fork and extract what you need for a PR. I'm afraid I don't have the spare time available to me to do this anytime soon.

[MarkSonghurst]

Resolved with #104