Feature request -- async version of blocking functions about node-nanomsg HOT 18 OPEN

I'm currently working on async bind and connect. https://github.com/comick/node-nanomsg/tree/async
Maybe we can discuss here which features it should have.

from node-nanomsg.

@comick , if you make the function signatures variadic, we can avoid making breaking API changes:
ex.

changing the currently sync:

Socket.prototype.bind = function (addr) {

to be async:

Socket.prototype.bind = function (addr, callback) {

and providing a sync method:

Socket.prototype.bindSync = function (addr) {

means that consumers of the bind API today will have breaking changes. This could be fixed by a major version increment.

Or:

we could avoid that by checking the length of args and calling the sync version if arguments.length === 1 else calling the aysnc version.

That way the last argument is optional, and the function signature looks more like:

bind(addr, [callback])

Thoughts?

from node-nanomsg.

With your proposed solution if a callback is given as second argument undefined is returned and callback is called at some point. Without the second argument instead the call will be implicitly sync.

Being sync or async is such a deep difference to make it dependent of how a function is called.
Node core for example have different versions for sync and async io functions.
Also zmq bindings for node have two different functions.
Do we have similar examples in other apis?

from node-nanomsg.

or, we could make the breaking changes, have a major version bump, check the length of arguments and assert that the typeof the callback arg is a function, else print a deprecation warning and call the sync version on their behalf. or throw.

from node-nanomsg.

With your proposed solution if a callback is given as second argument undefined is returned and callback is called at some point. Without the second argument instead the call will be implicitly sync.

It's correct that api would break without this solution. A major version increment would also make sense in this case. nanomsg today should not be used in production, I don't see risks here.
A deprecation warning can make sense for one major release cycle at least.

Being sync or async is such a deep difference to make it dependent of how a function is called.
Node core for example have different versions for sync and async io functions.
Also zmq bindings for node have two different functions.
Do we have similar examples in other apis?

from node-nanomsg.

nanomsg today should not be used in production, I don't see risks here.

I believe this library to be production ready. @tcr 's company is using it.

from node-nanomsg.

two interesting points raised by the discussion here. I would address each as follows:

• this library is production ready. we are starting to use it at my company
• both nn_connect() and nn_bind() each are synchronous c library operations. wrapping an async javascript function on top seems excessive.

from node-nanomsg.

On the first point, that's a fact, nothing to say :).
On the second point, you are right, but synchrounous is not a good idea in node. It takes some time and in the meanwhile you r processing is locked. Also, after your callback is called and no error is reported, at that point it makes sense to start your logic (send/receive).

Another option is to have bind with current behaviour and add bindAsync which works asynchronously. That's not really node style but wouldn't break compatibility.

Personally I'm for deprecation now with fallback based on missing parameter and dropping at some point in the future.

from node-nanomsg.

Maybe if we had a benchmark showing the throughput gains to be had by switching to async, this would be a no-brainer.

from node-nanomsg.

how is this coming along: https://github.com/comick/node-nanomsg/tree/async ?

After reviewing this work on @comick's tree today, and giving this API some second thought, I believe we should get this committed asap.

Being sync or async is such a deep difference.. Node core for example has different versions for sync and async io functions. Also zmq bindings for node have two different functions. Do we have similar examples in other apis?

please PR, and note that the callback to bind() and connect() should be optional, no need to revise current methods or add new bindAsync() or connectSync() methods:

push.bind('tcp://eth0:64000') //this is what we have now
pull.connect('tcp://123.45.678.890:64000', function() {
  console.log('connected'); //handshake complete 
})

preferred implementation approach: ensure flow control splits between respective native methods with a test for the presence of a second parameter to bind() and connect(), for example when it's undefined or in the alternative when arguments.length is greater than one, or if you use some other reliable way to figure that out that would be cool too.

from node-nanomsg.

bind implementation is done and tests work. connect implementation is mainly a matter of copy and paste and testing.
The optional parameter way is not the best approach in my opinion.
Given that:

• is good for libraries to use apis which are familiar to the core apis (that is async by default and callback where first param is return value and second is error state)
• breaking existing code is not nice for users

I strongly support nick's idea to run sync when the callback is missing, while raising a warning, so that the user can refactor from next version. Then, say from 0.4, drop warning and default to async.

Once we agree on that, we can have async bind and connect ready for merge.

from node-nanomsg.

i dont think we should raise a warning when the callback is missing because we shouldn't enforce our way of doing things on the user.

also, having an optional callback does not, nor should it, break any existing code. As for following mainstream APIs, if our API is easier to use than something familiar that's fine.

@nickdesaulniers thoughts?

from node-nanomsg.

The one thing I'm worried about is if people might confuse the APIs, for example:

var nano = require('nanomsg');

// Sync
var pub = nano.socket('pub');
var addr = 'tcp://127.0.0.1:7789'
pub.bind(addr); // sync
pub.send("Hello from nanomsg!");

// Async
var pub2 = nano.socket('pub');
pub2.bind(addr, function () {
  pub2.send("Hello from nanomsg!");
});
pub2.send("hello"); // What happens here???

I guess if you call send on a non bound socket, we don't throw, which is sub optimal. In the case where someone invokes bind async, we should mark the socket as async bind and error if the user calls send before being bound.

@comick , I don't want to deprecate existing APIs. I think new developers aren't expecting a matching API to node's net module, and I get really pissed off tracking down deprecation warnings and I'm sure existing users of the lib would too. It should be easy to do:

Socket.prototype.bind = function (addr, cb) {
  if (typeof cb === 'function') {
    // Async
    nn.Bind(this.binding, addr, cb);
  } else {
    // Sync
    return this._protect(nn.BindSync(this.binding, addr));
  }
};

from node-nanomsg.

@reqshark following the mainstream APIs is not only a matter of taste. It's all about plugging effertless into a well known world with libraries. Take as an example node libraries that promisify and depromisify. Consistency is important, specially in bigger projects.
Nevertheless, I'm going to pull request the binding part (as in @nickdesaulniers 's comment) soon.

from node-nanomsg.

Hey guys -

First of all, thanks for this library! You guys are doing an awesome job!

I'm working on Meshblu, an open internet of things platform, and we're thinking about using nanomsg in a performance-optimized core we're experimenting with.

The only thing that's really keeping me from recommending this library is the lack of asynchronous functions for connect and other methods you'd expect from a node library.

In our case, we will have a (potentially) very large number connections per server. The trouble with synchronous calls is that the call will hang the entire server thread until the operation is completed.

Since Node is traditionally single-threaded, this will cause the entire service to stop every time a client connects to it, until the connection is completed.

And if connections are opening and closing all the time...you get the picture.

Async code can definitely be a pain, but it's one of the features that made Node as popular as it is today - by relying on callbacks to resume control flow once slow (typically I/O) operations are completed, you can handle a lot more clients on a single thread.

We actually make a lot of our APIs asynchronous, even when they don't need to be. Especially when they might do something like request data from a database, write or read from a file, send data over a socket, or anything else asynchronous in the future.

In any case, thanks for all your hard work!

I just wanted to add my $0.02 on the async debate, and how important it is - It's a lot more than just a matter of taste.

from node-nanomsg.

Ciao @redaphid, I started an async implementation in a branch at https://github.com/comick/node-nanomsg/commits/async

Is not difficult to implement and was working well. Now I do not have any time to work on the project so it is as it is, at least bind is async, connect not yet, but same concepts apply. If you want to improve, fork from current master (not my fork) and use my fork's branch as inspiration.

It's all about some giving uv some job and being notified when it's done.

from node-nanomsg.

@comick Cool! Yeah, I was looking into how to do this, and it seems like libuv is the way everyone else does it.

from node-nanomsg.

+1000 for this, very important for Node.js

from node-nanomsg.