Finally hook errors?

[justinabrahms]

What happens when a finally hook errors?

Here's some example evaluation logic:

try {
  run_before_hooks()
  get_value()
  run_after_hooks()
} catch() {
  run_error_hooks()
} finally {
  run_finally_hooks()
}

If the finally hooks throw errors, they won't be caught which breaks 1.19 from the spec.

Otherwise, our code looks like this (which is very janky).

try {
  try {
    run_before_hooks()
    get_value()
    run_after_hooks()
  } catch() {
    run_error_hooks()
  } finally {
    run_finally_hooks()
  }
} finally {
  run_error_hooks() # again?
}

[toddbaert]

I think catch should have run_error_hooks() as well, (maybe I'm confused). We also mentioned that throwing in an error hook would also be bubble up that error, which would be another exception to 1.19 we should probably note in the spec.

I feel less sure about what the behavior should be in finally hooks. I think I could go either way on it, but it should certainly be documented. I suppose you could think of it like try/catch/finally in most languages, which will bubble up an exception if you throw in finally... so that might be a reasonable choice for us.

See https://github.com/open-feature/sdk-research/blob/main/packages/openfeature-extra/src/lib/hooks/open-telemetry-hook.ts for a real life example of all the hook life-cycle events.

[justinabrahms]

Updated the error_handling to run_error_hooks, which should make it clearer.

[toddbaert]

We might want to close this for now. Both the Java and Node SDKs throw errors from finally and error hooks it seems... which IMHO is fine. Hooks are "user defined" code, and I think throwing it them is reasonable in certain cases, particularly finally and error. Especially in the case of errors, it actually provides a mechanism to force errors to bubble up for debugging purposes, which seems like it could be useful in dev environments.

[justinabrahms]

I think we should write the decision in the spec, regardless of what it is. I can codify the throwing into the spec soon if someone doesn't beat me to it first.

On Fri, May 27, 2022, at 9:01 AM, Todd Baert wrote:

We might want to close this for now. Both the Java and Node SDKs throw errors from finally and error hooks it seems... which IMHO is fine. Hooks are "user defined" code, and I think throwing it them is reasonable in certain cases, particularly finally and error. Especially in the case of errors, it actually provides a mechanism to force errors to bubble up for debugging purposes, which seems like it could be useful in dev environments.

— Reply to this email directly, view it on GitHub https://github.com/open-feature/spec/issues/38#issuecomment-1139750441, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAA6DIXU5AI2MSJ3CRXYMLVMDWVHANCNFSM5UJAZDPQ. You are receiving this because you authored the thread.Message ID: @.***>

[toddbaert]

It seems to me this is partially already in the spec (4.3): https://github.com/open-feature/spec/blob/main/specification/flag-evaluation/hooks.md#requirement-43

But we should also say something similar about error I guess?