Skip to content

Conversation

@QinanLi
Copy link

@QinanLi QinanLi commented Oct 13, 2016

This PR adds a context.Context to the options struct which allows easy cancellation of running scripts.

As context was only introduced in Go 1.7 this would mean dropping support for previous versions of Go.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 90.449% when pulling 4aec3cb on xencode:add-context into d0d5dd3 on yuin:master.

@yuin
Copy link
Owner

yuin commented Oct 13, 2016

I'm afraid this would be difficult to accept.

  1. GopherLua is already used in many projects, so silently dropping older versions is going to break these projects.
  2. This functionality is implemented at debug.sethook in original Lua implementation. And debug.sethook is not implemented in GopherLua because it cause a little performance degradation and is not quite so common. I suspect this PR causes a performance degradation too.

@yuin
Copy link
Owner

yuin commented Jan 18, 2017

@QinanLi , I've added a context support for GopherLua using "golang.org/x/net/context" .
golang.org/x/net/context has a fallback mechanism, so you can use same code for Go1.5, Go1.6 or Go1.7.
If you use Go1.7, golang.org/x/net/context uses builtin context package as its implementation.

And I completely separated VM execution path of the case using the context and the case not using the context. This means there is a no performance degradation when using GopherLua without a context.

@QinanLi
Copy link
Author

QinanLi commented Jan 18, 2017

@yuin that's great! It'll be good news for others with the same problem but I had a less restrictive use case that required less than the rework that you've implemented. All the same thanks a lot for your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants