#105 - Panic Handlers

[jmickey]

What this PR does / why we need it:

Allow lib callers to register panic handlers, and choose decide if panics will continue to cause the code to crash.

A defer function has been added to the call sites where panics are made in the library code. Initially I was trying to follow the references up the stack to see if there were fewer places where a defer could be made, but in the end it made the more sense to place the defer close to the panic itself.

Lib consumers can register their own handlers that matches the PanicHandler type - func(interface{}):

import "github.com/open-component-model/ocm/pkg/util/panics"

func main() {

	panics.RegisterPanicHandler(func (in interface{}) {
		// do something
		return true
	})

}

By default panics will always log the stacktrace.

Which issue(s) this PR fixes:
Fixes #105

[Skarlso]

Nice work! :)

[mandelsoft]

I'm wondering how (and why) you have selected the locations to place the defer. It looks like you have chosen places near the occurrence of panics. But this typically leads to a continuing program flow with undefined results, where the caller has no possibility to identify this case, which basically will lead to further runtime panics. These panics are for sure NOT handled by a panic handler, again, so that you still get panics, but now completely unrelated to the original problem.

Similar to the K8S handling, the defer should be placed near a resurrection point in a program flow, where typically a loop for request handling is in place to assure the handling of further requests, even if a single single one runs in such a problematic situation (for example, reconcilation loop). Or at least a place, where you can safely ignore the outcome of a called function and continue with a new calculation context.

A defer inside an ìnit function is questionable. If the program initialization fails because of some basic misalignment, it makes no sense to run the process at all.

The Must versions of functions panic if the non-Must versions return an error.
Placing defers here is also questionable, because you could directly call the error providing function if you want to deal with failures. The Must version is typically intended to be called in init functions (see above).

[jmickey]

I'm wondering how (and why) you have selected the locations to place the defer. It looks like you have chosen places near the occurrence of panics.

Correct. Restricted to panics in the library code.

But this typically leads to a continuing program flow with undefined results, where the caller has no possibility to identify this case, which basically will lead to further runtime panics. These panics are for sure NOT handled by a panic handler, again, so that you still get panics, but now completely unrelated to the original problem.

I would argue that this isn't our concern, but rather the users concern. Optimally we wouldn't panic from library code, but because we do, we should at least provide the option to handle those panics as the user sees fit. I would imagine that in the vast majority of cases the crash will continue. If a user opts to continue the program flow then the consequences of that are for them to deal with.

Similar to the K8S handling, the defer should be placed near a resurrection point in a program flow, where typically a loop for request handling is in place to assure the handling of further requests, even if a single single one runs in such a problematic situation (for example, reconcilation loop). Or at least a place, where you can safely ignore the outcome of a called function and continue with a new calculation context.

I'm not really sure I understand what your saying here, and how it relates in the context of OCM and defer placements.

A defer inside an ìnit function is questionable. If the program initialization fails because of some basic misalignment, it makes no sense to run the process at all.

If you feel strongly about this I can remove the defers from inits. Panicking from an init is, at least to me, also questionable unless that init sits in code that is wholly owned by an executable context rather than lib context (e.g. a cmd executable).

The Must versions of functions panic if the non-Must versions return an error.

The user still may wish to execute some process or code before continuing with a crash, so I think it is still valid to call the handlers here?

[Skarlso]

We'll do a compromise and place handling at certain points only. The rest will be handled in a different manner.

The ADR has been accepted by all parties so we'll try to be more in line with that. :)

[In-Ko]

@jmickey / @Skarlso / @mandelsoft : Can we please get this clarified this week ? It is a long runner already, and I'd like to see this being finished soon. My take: If the current implementation is done according to the ACD which we all agreed on, than this should go out.

[Skarlso]

Right, so the main points are:

• The problem with catching a panic where they are is that somewhere along the line another panic will happen because now the result is undefined or even nil so the resulting error will be unrelated to the first panic causing problem.
• Put the handler into a place of recovering.
  Regarding this point.... There are no safe places in OCM after a panic. There are lot of places where error handling is non-existent so this is at least not really possible without significant changes. I guess in those cases, we will just ignore the panic. I don't have an alternative.
• Remove the defer in init
• Ignore MUST functions.

Please re-review with the new changes.

[mandelsoft]

Good job, to find all the places where a regular error return can do the job to avoid panics at all.

One panic should be avoided by refactoring the interface and add an error return value.
If you want I can do this at short notice. There are probably many places involved.

[Skarlso]

@mandelsoft This looks good to me now. :) Thanks for the additions. 👍

Feel free to merge at your earliest convenience. :)