Implements Mrb.ClearException

[mikesimons]

Mruby will set state.exc to the last exception that occurred but this field is never cleared by go-mruby, nor is there a way for calling code to do so.

Since all codepaths check this field after executing ruby code to determine if there was an error, all attempts to run code in a VM after an exception occurs will result in the same exception.

The idiomatic mruby way of handling exceptions seems to be to clear state.exc and either carry on or invoke backtrace and such on the stored exception.

This fix just exposes a way to clear the field so that inspection of the exception object can occur (for backtrace and such) or it can simply be ignored.

[mitchellh]

Ah weird, I never knew about this.

I think what I'd rather do is introduce a helper to check m.state.exc and return an error (Go type) properly, and it handles clearing it on its own. That way whenever we check for an exception it is automatically cleared. What do you think about that?

[mikesimons]

Funny you should mention that; I have the following helper already implemented (based on this ClearException PR & a patch to get the MrbValue out of an Exception which wouldn't be required if it were in the mruby package)...

type MrbException struct {
    File      string
    Line      int
    Message   string
    Backtrace []string
}

func exceptionToMrbException(mrb *mruby.Mrb, exc *mruby.Exception) *MrbException {
    mrb.ClearException()
    excMrbVal := exc.ToMrbValue()

    result := &MrbException{}

    mrbBacktrace, _ := excMrbVal.Call("backtrace")
    mruby.Decode(&result.Backtrace, mrbBacktrace)

    if msg, err := excMrbVal.Call("message"); err == nil {
        result.Message = msg.String()
    }

    if len(result.Backtrace) < 1 {
        result.File = "unknown"
        result.Line = 0
    } else {
        fileAndLine := strings.Split(result.Backtrace[0], ":")
        result.File = fileAndLine[0]
        result.Line, _ = strconv.Atoi(fileAndLine[1])
    }

    return result
}

Please excuse the nasty name; I was feeling lazy 😛

Is that the kind of thing you mean?

If so, happy to refactor it + add tests to sit inside mruby. MrbException could be merged in to mruby.Exception which would probably be a bit tidier. Can't think of a reason not to do that.

[mikesimons]

Second thoughts, that function above is probably not what you meant. The common pattern in sites that check state.exc is:

if m.state.exc != nil {
    return nil, newExceptionValue(m.state)
}

return newValue(m.state, result), nil

I guess you could turn that in to something like:

return mrubyExecReturn(m.state, result)

Where mrubyExecReturn would look like:

func mrubyExecReturn(state *C.mrb_state, result C.mrb_value)  (*MrbValue, error) {
    if state.exc != nil {
        exc := newExceptionValue(state)
        state.exc = nil
        return nil, exc
    }
    return newValue(state, result), nil
}

I think that was more of what you meant. Though I still wouldn't mind seeing the exception helper func integrated too 😉

Happy to tackle either if they sound good to you.

[mitchellh]

Mike, that sounds right. :) I wanted a helper to just do the exc check for me and return that. I think I'd actually make it like this (unformatted):

func checkException(state *C.mrb_state) error {
    if state.exc == nil { return nil }
    err := newExceptionValue(state)
    state.exc = nil
    return err
}

So it isn't a function to "return this or that", it is just a standard idiomatic Go function to just check for error.

[mikesimons]

Aha; I was over-complicating it. Nice.

Would you be interested in entertaining the addition of backtrace / file / line (to mruby.Exception) as part of newExceptionValue too?

For my particular use-case I'm taking user supplied erb which may contain syntax / logical errors that I need to be able to report.

I imagine that most use-cases of go-mruby would be for extension / configuration which would similarly benefit.

[mitchellh]

Mike, I think your idea is great. I think adding Line and Backtrace and such to Exception is a good idea. Instead of doing what you're doing, I'd make the calls lazy so we don't have any extra Go-to-C-to-Ruby overhead. So, if the user calls Line() then we can ask for the line then.

These are two separate ideas so if you end up going for it then please make two separate PRs! Thanks

[mikesimons]

👍 Will close this one off, work on the exception helper and then the Exception extensions.