use libuv thread pool for blocking functions
Closed this issue · 45 comments
All the functions doing IO should use libuv's uv_queue_work function in order to not block Node.js main thread.
Some useful references:
- Libuv API: https://github.com/joyent/libuv/blob/master/include/uv.h
- Libuv book section about uv_queue_work: http://nikhilm.github.io/uvbook/threads.html#libuv-work-queue
how do you want to implement this? all functions will be async or only some?
I'd think that all of them have to be async since most drivers in libvirt are likely to block nodejs main thread, but I could be wrong.
How about making a node-libvirt-async npm module that is basically a wrapper for all the functions, calling the node-libvirt functions in process.nextTick() and calling the provided callback once that finishes?
That way making node-libvirt a real async library won't mean an API change...
We could try but I'm not entirely sure it will have the effect you describe. CPU bound code will benefit better from process.nextTick. Libvirt functions are mostly I/O bound and it doesn't seem like process.NextTick will help a lot there.
I'll get started on this.
Sounds like the sufficient API change for this would be for functions that do blocking IO to take callbacks rather than returning values -- correct?
I've unfortunately not yet had a chance to do more than look through the code and start to search for blockacious functions. I'm still interested in working on this, but I don't want to hold the show up if someone else gets to it first.
Right, callbacks everywhere. Additionally, passing error objects instead of throwing exceptions.—
Sent from Mailbox
On Tue, Jul 8, 2014 at 3:14 AM, Steven Dee notifications@github.com
wrote:
Sounds like the sufficient API change for this would be for functions that do blocking IO to take callbacks rather than returning values -- correct?
I've unfortunately not yet had a chance to do more than look through the code and start to search for blockacious functions. I'm still interested in working on this, but I don't want to hold the show up if someone else gets to it first.
Reply to this email directly or view it on GitHub:
#9 (comment)
Alternatively, return Promise/A objects which handle asynchronous throws,
can be chained and cached and are becoming the de facto standard for
asynchronous javascript...
That is correct.
@c4milo Would like to work on this. To confirm, something like this should be supported on all functions?
hypervisor.getVersion(function(version) {
assert.isNotNull(version);
});and
hypervisor.getVersion().then(function(version) {
assert.isNotNull(version);
});Hey,
@vegetableman , did you start working on it ?
I'm working on a project where an application need to handle a lot of different libvirt connections, and one cannot lock the node process if it has a problem (or not ...). So i'm looking to use libuv workers to call libvirt C functions (i think all functions will need it, because there're all likely to talk to a remote hypervisor)
Thanks
Anthony.
Nope. Was expecting a reply for the above query to give it a shot for the bounty. Didn't receive any :(.
@atoy40, @vegetableman I'm sorry I couldn't answer on a timely fashion. I've been swamped. I'm happy to give you access to the repo if you want to work on these issues.
@vegetableman regarding your question: I worked under the assumption that hypervisors should always report some sort of version. If it is not true, feel free to get rid of that assertion.
Just started to work on it this afternoon (because i've a lot of problems with my app, for example when a connection wait for a ssh key authorization... the whole app is locked)
Just to test, i started to async the connection method (now need a connect() method instead of connecting trough the constructor as it is in the current synchronous version) and the getCapabilities() method.
I've commited here atoy40/node-libvirt@0a6d0f3 (connect) and here : atoy40/node-libvirt@725502f (getCapabilities)
and tested with a simple piece of code. Works perfect :)
var Libvirt = require("../node-libvirt");
var hv = new Libvirt.Hypervisor("qemu:///system");
hv.connect(function(err) {
if (err) {
console.log("Unable to connect : "+err.message);
return;
}
console.log("connected !");
hv.getCapabilities(function(err, res) {
if (err) {
console.log("Unable to get capabilities : "+err.message);
return;
}a
console.log("XML Capabilities : ");
console.log(res);
});
});Before continuing, i've to think a bit about the best ways to factorize and minimize code changes (ideas are welcome :)).
Anthony.
@atoy40 that looks good to me. It would be better if we can avoid callbacks, though. Would it be possible to use ES6 semantics, promises perhaps?
I think it can be done using some JS shims, so you'll not use directly the C++ object/wrapper.
The wrapper itself has to be traditionnaly async, with callback, then a JS library can expose it using a promise/A implementation
Sounds good, although, I think newest V8 versions have native support for promises. Do you think it will be better to do it with JS shims?
+1 for returning promises if possible. Otherwise exposing object prototypes so it can be easily promisified would work, too.
It may be outside the scope of this issue and more related to #33, but if a large rework is taking place I would also recommend the Native Abstractions for Node library to provide future compatibility with node 0.12+ and io.js. The library also has a NanAsyncWorker class that simplifies async queuing/handing.
Absolutely, thanks for bringing it up, using NAN would be ideal to make migrating to future node.js/io.js versions more seamless.
@thwi , i cannot show you my browser history, but it's full of nan.h since yesterday :D
Now using NAN to handle async worker.
https://github.com/hooklift/node-libvirt/compare/master...atoy40:async?expand=1
@c4milo have you any pointers about new v8 and promise support ? because i've read promise implementation are outside of node/io.js engine, so you can have many different implementations. This is why i think a JS shim between apps and c++ wrapper is the easiest (and best ?)
https://code.google.com/p/v8/source/browse/trunk/src/promise.js
I must say, though, that I've never use them before. There are people
saying they were slow and a moving target (
http://stackoverflow.com/a/21754176), however I'm not sure if that still
holds true nowadays.
On Fri, Feb 27, 2015 at 8:45 AM Anthony Hinsinger notifications@github.com
wrote:
@c4milo https://github.com/c4milo have you any pointers about new v8
and promise support ? because i've read promise implementation are outside
of node/io.js engine, so you can have many different implementations. This
is why i think a JS shim between apps and c++ wrapper is the easiest (and
best ?)—
Reply to this email directly or view it on GitHub
#9 (comment).
Promises are completely awesome. They make async programming easy, allow
you to throw in async context, give you a object to cache responses (both
answers and failures) with, and generally clean up the code. They are of
course slower than not doing anything, but their overhead is generally
dwarfed by the async things you are wrapping with them.
On Fri, Feb 27, 2015 at 3:35 PM Camilo Aguilar notifications@github.com
wrote:
https://code.google.com/p/v8/source/browse/trunk/src/promise.js
I must say, though, that I've never use them before. There are people
saying they were slow and a moving target (
http://stackoverflow.com/a/21754176), however I'm not sure if that still
holds true nowadays.
On Fri, Feb 27, 2015 at 8:45 AM Anthony Hinsinger <
notifications@github.com>
wrote:@c4milo https://github.com/c4milo have you any pointers about new v8
and promise support ? because i've read promise implementation are
outside
of node/io.js engine, so you can have many different implementations.
This
is why i think a JS shim between apps and c++ wrapper is the easiest (and
best ?)—
Reply to this email directly or view it on GitHub
<#9 (comment)
.—
Reply to this email directly or view it on GitHub
#9 (comment).
@wmertens Oh, my bad, in my previous comment I wasn't putting in doubt the usefulness of promises. I think we all agree that will be the best way to go. The question is whether we use V8's native promises or something like Bluebird.
Given that any promise library can consume the native promises, native
seems like the way to go, but of course it limits you to recent Node
versions. Not that I think that's a problem.
On Sat, Feb 28, 2015 at 1:00 AM Camilo Aguilar notifications@github.com
wrote:
@wmertens https://github.com/wmertens Oh, my bad, in my previous
comment I wasn't putting in doubt the usefulness of promises. I think we
all agree that will be the best way to go. The question relates to whether
we use V8's native promises or something like Bluebird.—
Reply to this email directly or view it on GitHub
#9 (comment).
Hey, i've migrated all Hypervisor methods using GET_LIST_OF and GET_NUM_OF (so ~20 methods in one shot :D), and also Domain::lookupByName(). Then to test usability, i've written a little piece of code using bluebird promise implementation and it looks really nice and easy to use. I've simply "promised" 4 methods, and it become :
connectAsync()
.then(function() {
return getDefinedDomainsAsync();
}, console.err)
.map(function(domname) {
return lookupDomainByNameAsync(domname);
}, console.err)
.each(function(dom) {
console.log(dom.getInfo());
}, console.err);connect ... THEN get domain names THEN get lookup each domain THEN log domains info for each one.
Anthony.
It's awesome to see people working on async node-libvirt. I think Promises are the right way to go but bindings should not be concerned with promises to not complicate things. There should be some thin JS layer on top of native layer with magic like https://github.com/petkaantonov/bluebird/blob/master/API.md#promisepromisifyallobject-target--object-options---object
Basically almost any callback based function can be "promisified".
yes @Rush, this is exactly what i'm trying to do : let the binding dealing with "standard" callbacks (standard means error/undefined as first parameter) and write a simple JS wrapper over it to returns promises.
The first big piece (the Domain object) is damn finished !!! :)
I'm starting the Hypervisor, another big one, then the others are smaller.
Anthony.
@c4milo @atoy40 others maybe: I think it's probably time I signal my intent 😄 I've got a fork of this project that converts the entire module to work asynchronously over here. I'm still trudging through the Domain class, but everything else has been converted. The branch builds on @atoy40 's work, and good lord do I understand his pain: the word "tedious" doesn't begin to explain this process.
There are a number of things that may or may not be appreciated/accepted, and I usually try to do things very by-the-book but I'm just very pressed for time and need this functionality ASAP:
- Things that fall into the "need to be ironed out"/"people should fix it if/when it's a problem" bucket:
- I've temporarily removed all of the Domain events because I think they should live in a better place, and generally had a rough implementation (these should be using an EventEmitter at very least, will get to it later..)
- I've removed all the static Persistent strings as they were needlessly being kept around. If you really need to keep these things defined at the top of the file (perceivably to make breaking api changes in the future I guess?) they should be #defines or const char*s.
- I have followed in the tradition of basically ignoring all flags for all virFunctions atm. This allowed me to get further more quickly, and I still think the number of approaches used in the existing codebase could use some work
- Bigger changes someone might get upset about 😄
- I'm using c++11 explicitly so I can use std::shared_ptr. Again, I don't have time to implement this myself, and it provided a lot of flexibility when implementing the LibVirtHandle
- I've converted all of the tests to mocha+chai. I'm more used to it, async testing flows naturally with that combination for me. I've got no more excuses here.
- I completely changed the style of the C++ files. Again I have no real excuse here, other than I did in fact touch every single line of code in this project, and the style just made it easier for me to visualize huge swaths of code on my 13" macbook screen
NOTE: this is most decidedly a "breaking change", but then again I think that was in order for the module in the first place. I'm not sure whether you're even interested in the code, in which case I'm perfectly happy to leave it as fork.
Promises: now that everything is converted to a standard node-callback format, slapping on bluebird Promisification is easy as pie. The tests probably look very cumbersome right now, but with promises that will all look much nicer.
OK, that's it, hopefully we can get some discussion going!
@mbroadst: Sounds good. I like your C++11 approach. I think this should simplify lots of the code. I went for similar approach when doing node-cbind (https://github.com/encharm/node-cbind) but I didn't get to do the interesting stuff yet due to lack of free time. :) We will get to test your branch soon enough. Thanks for all the hard work.
@Rush Cool. LibVirtHandle is the only thing that needs C++11 features, and it can use a LOT of work - it's pretty slapped together but it works for the moment. The intent was to make a shared union type for all the different pointers. It uses a union in its dptr, but frankly it could probably be stored just as a single void*, and provide a templated .As<>() accessor (which would make the macros/template helper worker classes all that much cleaner). Again, I have so very little time here so this is what I've got for today 😄
@Rush another thing I haven't quite handled is that test order still matters unfortunately. This seems to be a bug in the libvirt test driver itself, as there doesn't seem to be an easy way to forcibly reset the internal state (the driver's local storage is "per-process" so we're SOL when running all tests through a single mocha run). As a result make test will pass ~108 tests, but not run any of the StoragePool tests, however if you run the StoragePool tests by themselves they all pass.
@mbroadst brave efforts, I generally like the direction you are taking. Node-libvirt was originally written in 2010, following the same C++ code conventions Node was following which at the time I think were Google C++ conventions. That said, my only concern with your changes is removing features some users might be currently using like Domain events.
I've removed all the static Persistent strings as they were needlessly being kept around. If you really need to keep these things defined at the top of the file (perceivably to make breaking api changes in the future I guess?) they should be #defines or const char*s.
The main reason for persistent handlers was so that the V8's GC wouldn't remove them. Things are now different, we should be aware of https://strongloop.com/strongblog/node-js-v0-12-c-apis-breaking/ while doing the refactoring.
I have followed in the tradition of basically ignoring all flags for all virFunctions atm. This allowed me to get further more quickly, and I still think the number of approaches used in the existing codebase could use some work.
Generally agreed with you as well. But again, removing features is the only thing that concerns me.
I'm using c++11 explicitly so I can use std::shared_ptr. Again, I don't have time to implement this myself, and it provided a lot of flexibility when implementing the LibVirtHandle
I don't mind this at all as long as @atoy40 agrees with it too.
I've converted all of the tests to mocha+chai. I'm more used to it, async testing flows naturally with that combination for me. I've got no more excuses here.
This is lovely 👍
I completely changed the style of the C++ files. Again I have no real excuse here, other than I did in fact touch every single line of code in this project, and the style just made it easier for me to visualize huge swaths of code on my 13" macbook screen
This doesn't bother me either as long as the code gets simpler, cleaner and remains consistent throughout.
Promises: now that everything is converted to a standard node-callback format, slapping on bluebird Promisification is easy as pie. The tests probably look very cumbersome right now, but with promises that will all look much nicer.
Thanks for all the hard work! 💃
@mbroadst I'm going to add you as contributor to the project if you don't mind. This project needed this revamp, I even wanted to do it myself but I found less and less time to work on it :/
@c4milo re: the Persistent stuff. What I meant specifically was I've removed all the static Persistent handles to strings that are used as identifiers in returned objects (see here for example). This was needless, and V8 isn't going to reclaim the memory. Things that indeed need to be persistent are still persistent 😄
As far as removing features, there are only a handful of breaking forward facing API changes. I will reintroduce the domain events, and I don't imagine those method names will change (but you will probably have to listen to EventEmitter events instead of using a callback). A few of the method names have changed where they made more sense ("GetListOfInterfaces" => "ListInterfaces" type stuff).
I understand that it's an inconvenience to new users, but I also think you can easily peg a version so that they can maintain their existing code while we can actually move forward. I imagine this code should represent a major bump when its complete.
@c4milo re: the Persistent stuff. What I meant specifically was I've removed all the static Persistent handles to strings that are used as identifiers in returned objects (see here for example). This was needless, and V8 isn't going to reclaim the memory. Things that indeed need to be persistent are still persistent 😄
👍
As far as removing features, there are only a handful of breaking forward facing API changes. I will reintroduce the domain events, and I don't imagine those method names will change (but you will probably have to listen to EventEmitter events instead of using a callback). A few of the method names have changed where they made more sense ("GetListOfInterfaces" => "ListInterfaces" type stuff).
I understand that it's an inconvenience to new users, but I also think you can easily peg a version so that they can maintain their existing code while we can actually move forward. I imagine this code should represent a major bump when its complete.
I'm in favor of using EventEmitter too. I also like a lot the API cleaning. You are right, we can just release a new version, making explicit that it breaks backwards compatibility.
guys I think its time to close this issue. The current master passes something like 150+ tests all using async callbacks. There are parts of the API that are not yet converted, and will be on a case-by-case (best effort) basis, but the general support is there.