Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Attach last error to the state #70

Closed
wants to merge 2 commits into from
Closed

Attach last error to the state #70

wants to merge 2 commits into from

Conversation

andrewhare
Copy link

This fixes #24 by adding a lastErr attribute to the state, we can track the error that was received by the error handler while still allowing the error handler function to return. Since the function returns, the stack is properly cleaned up and both the "C stack overflow" and "error in error handling" errors go away.

This fix also improves performance:

Before:

BenchmarkFix-8          10000        131910 ns/op       45529 B/op         897 allocs/op
PASS
ok      github.com/andrewhare/tmp    1.340s

After:

BenchmarkFix-8         300000          4258 ns/op         552 B/op          14 allocs/op
PASS
ok      github.com/andrewhare/tmp    1.348s

Running the following benchmark:

package main

import (
    "testing"

    "github.com/andrewhare/golua/lua"
)

var script = `
function doit()
    x[1] = 2
end
`

func BenchmarkFix(b *testing.B) {
    L := lua.NewState()
    L.OpenLibs()
    if err := L.DoString(script); err != nil {
        panic(err)
    }
    for n := 0; n < b.N; n++ {
        top := L.GetTop()
        L.GetGlobal("doit")
        err := L.Call(0, lua.LUA_MULTRET)
        if err == nil {
            panic("should have error")
        }
        L.SetTop(top)
    }
}

@andrewhare andrewhare mentioned this pull request Jun 6, 2019
Open
@aarzilli
Copy link
Owner

aarzilli commented Jun 7, 2019

Lua errors can't be allowed to complete, otherwise they'll end up executing a longjump, which will skip over a bunch of go frames, which isn't allowed by go runtime. See CI failure.

@andrewhare
Copy link
Author

andrewhare commented Jun 17, 2019 •
edited
Loading

@aarzilli I've pushed an experimental approach, basically wrapping each API call in pcall. While this solves the segfault, it still doesn't bubble the wrapped call error back up the way the panic did before so this isn't complete.

I'm probably not going to put any more work into this than I have since my original fix works for my use-case (and I'm actively using a fork), so feel free to do whatever you want with this PR.

The idea behind this is approach that every C API call could be wrapped and then passed to pcall instead of being called directly.

@aarzilli aarzilli closed this Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

C stack overflow in error handler
2 participants
@andrewhare @aarzilli