Fix query unrecoverable panic

[amimart]

Issue description

When calling Interpreter.QueryContext(...) the prolog query is executed in another goroutine, as this is a different goroutine than the caller's one, if a panic occurs executing the query (i.e. typically executing a predicate) we can't recover from the caller's goroutine.

Proposed solution

The proposed fix moves the panic occurring in the querying goroutine to the Solutions.Next() call to allow the caller to recover.

The implementation simply recover any panic in the querying goroutine, sets the recovered error value to the Solutions structure to propagate the panic in the Next() method, if any.

Remarks

I tried to find the least invasive way possible, but I may not have seen the consequences of that choice, either in design or implementation. Please let me know if you have a more elegant way to handle this, I'd be happy to discuss it.

Thanks!

[ichiban]

Hi! Thank you for the high quality PR!

I think custom Go predicates should recover panics and turn them into prolog exceptions so that the caller can catch/3 them. This kind of treatments is already done in length/2 and functor/3 when make([]Term, n) is called with an absurdly large n.

    i.Register0(engine.NewAtom("do_not_call"), func(_ *engine.VM, k engine.Cont, env *engine.Env) (p *engine.Promise) {
        defer func() {
            if r := recover(); r != nil {
                p = engine.Error(fmt.Errorf("panic: %v", r))
            }
        }()
        panic("told you")
    })

?- catch(do_not_call, X, true).    
X = error(system_error,'panic: told you').

As for unforeseeable panics, we can do this kind of treatments in Predicate0.call() and (*Promise).Force() as a backup.

[amimart]

Hi! Thank you for your reactivity!

I think custom Go predicates should recover panics and turn them into prolog exceptions so that the caller can catch/3 them.

I agree with you custom Predicate should be panic free and I like the idea to use catch/3. However, even with provided predicate the panic free can't be ensured by design. Typically the Consult returning a delayed Promise, if built on top of a panicking file system is hard to make safe.

As for unforeseeable panics, we can do this kind of treatments in Predicate0.call() and (*Promise).Force() as a backup.

I thought about setting the recover in (*Promise).Force() but as it returns an error the panic any value needed to be wrapped in a dedicated error type, for instance:

type PanickedError struct {
    Value any
}

func (e PanickedError) Error() string {...}

I think introducing a new error type for this may be too much, what do you think ?

[ichiban]

Typically the Consult returning a delayed Promise, if built on top of a panicking file system is hard to make safe.

Could you elaborate on that? I've never thought of a file system panicking so if that's the case, we should recover it in consult/1 and convert it to a dedicated prolog exception.

Regarding the error type, I don't think PanickedError is too much as long as it's unexported (so, panickedError?). I also think fmt.Errorf("panic: %v", r) will suffice.

Anyways, I want to leave the option of re-panicking to the users of this library.

[amimart]

Could you elaborate on that? I've never thought of a file system panicking so if that's the case, we should recover it in consult/1 and convert it to a dedicated prolog exception.

That was just an example, a possibility to illustrate that it is difficult to guarantee panic free.

Regarding the error type, I don't think PanickedError is too much as long as it's unexported (so, panickedError?). I also think fmt.Errorf("panic: %v", r) will suffice.

I can implement it that way If you prefer, to be noted that the panic value won't be accessible by users of the library.

[ichiban]

That was just an example, a possibility to illustrate that it is difficult to guarantee panic free.

I see. Yes, there're many possibilities to panic and it's a good example other than predicates. Thank you for elaborating on that!

I can implement it that way If you prefer, to be noted that the panic value won't be accessible by users of the library.

Do you think the panic value should be accessible? If not abused, panics are for when the program cannot continue. There's little that can be done with it. So, oftentimes the panic value is simply a string.

[amimart]

Do you think the panic value should be accessible? If not abused, panics are for when the program cannot continue. There's little that can be done with it. So, oftentimes the panic value is simply a string.

To give a more concrete example here is the original motivation, we use this library at OKP4 in a constraint environment on which we must ensure the execution of Prolog programs doesn't exceeds a computation limit, and in order to be strict regarding such requirement the panic seems suitable to provide fail fast behavior.

The possibility to recover the panic value was just under the idea to provide the most open approach to give users as much freedom as possible.

Regarding the alternative you proposed https://github.com/ichiban/prolog/pull/290, I actually like the idea to convert it to a Prolog exception, it enhance the possibilities of panic handling, allowing to manage it either directly in Prolog or as a fail fast mechanism.

I think we may close this one in favor of https://github.com/ichiban/prolog/pull/290 :)

[ichiban]

Okay, let's proceed with #290 then! Thank you very much for the contribution!

[amimart]

Excellent thank you! It's a pleasure 😉