-
Notifications
You must be signed in to change notification settings - Fork 187
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.
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
Allow Coroutine Continuations #618
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor nitpicks for now, haven't looked at the implementation too much yet.
Just to help me wrap my head around this, could you provide some example code showing a use-case this change allows for (e.g. before, code with callbacks would be written like this, but now it could be written like this). |
Should add some tests to cover with coroutine or not. |
A quick and simple comparison would be using uv_fs_ (since its the largest consumer of uv_req_t). completely async: uv.fs_open(path, flags, mode, function(err, fd)
assert(fd, err)
-- do something with fd
end currently completely blocking: local fd, errstr, errname = uv.fs_open(path, flags, mode)
assert(fd, errstr)
-- do something with fd note: this would be a coroutine continuation if inside a non-main coroutine with coroutine continuation (yield + resume): local fd, errstr, errname = uv.fs_open(path, flags, mode, coroutine.running())
assert(fd, errstr)
-- do something with fd I attempted to make the resume of the coroutine match a blocking function call so that LUV_FORCE_COROUTINE_CONTINUATION can work seamlessly.
I definitely agree that this needs a lot more tests. Thats what adding coroutine.wrap()'d tests to tap was intended to accomplish; but it doesn't test the new surfaces that I introduced. |
Seems like the added test is failing on LuaJIT |
3b74dec
to
799bd08
Compare
I can't quite understand why exactly the appveyor build is failing. The failing tcp test passes on my machine on windows and there seems to be garbage at the bottom of the log? |
Allows passing a coroutine in place of a callback in all methods using uv_req_t. The coroutine will be yielded and then resumed using the synchronous argument list [not the (err, value) callbacks recieve]. I have not yet completely verified the behaviour when passing a coroutine that is not the running thread, however it should work. Enabled by default, any blocking function call inside of the not-main coroutine will be a yield + resume to allow the libuv event loop to continue running. This behavior can be changed with the LUV_FORCE_COROUTINE_CONTINUATION flag to cmake. Tap has been modified to *also* run tests inside of a coroutine to provide a testing suite for these changes. This is a merge of 5 commits: * add coroutine continuation support * add changes to documentation, add LUV_FORCE_COROUTINE_CONTINUATION * run test suite over all functions run in a coroutine * luv_error dislikes success statuses * fix memory leak in write when yielded Signed-off-by: truemedian <[email protected]>
82097c8
to
fb13c51
Compare
fb13c51
to
e2619c7
Compare
Closed because it makes continuations much more complicated. Similar behavior can be reproduced via: local thread = coroutine.running()
local req, err = uv.write(stream, function(...)
assert(coroutine.resume(thread, ...)) -- resume waiting coroutine, beware this will eat your stack trace
end
if not req then
return req, err -- write failed for some reason
end
return coroutine.yield() -- wait for write to finish |
Allows passing a coroutine in place of a callback in all methods using uv_req_t. The coroutine will be yielded and then resumed using the synchronous argument list [not the (err, value) callbacks recieve]. I have not yet completely verified the behaviour when passing a coroutine that is not the running thread, however it should work.
Disabled by default, any blocking function call inside of the not-main coroutine will be a yield + resume to allow the libuv event loop to continue running. This behavior can be changed with the LUV_FORCE_COROUTINE_CONTINUATION flag to cmake.
Tap has been modified to also run tests inside of a coroutine to provide a testing suite for these changes.
I have not run into any issues so far, but the luv test suite definitely does not cover all bases.