callback is called synchronously

[yortus]

Hi @adleroliveira,

Saw this at top of /r/node, congrats!

I noticed that dreamjs supports an async API but its implementation is fully synchronous. This will lead to errors in usage like the following:

dream.output((err, result) => {
  useResult(result);
});
var useResult = () => {
    // do something with result...
}

The async callback model of node guarantees that the callback won't be called until the next turn of the event loop (at the earliest), so the example should be 100% safe.

It looks like your implementation does everything synchronously and calls the callback synchronously too, so the useResult(result); line gets called before useResult is defined, and an error ensues.

More generally, there's no point offering callbacks/Promises/streams if the implementation is synchronous. The synchronous implementation completely starves the event loop until it finishes, so there is no opportunity for other code to run. I would suggest:

• Either: drop the asynchronous API since it's a fully synchronous implementation
• Or: change the implementation to break processing into smaller parts which are processed asynchronously by putting them individually on the event loop using eg setImmediate. When the full output is ready, then return the result through the callback/Promise/etc.

As for which is best, I'd say it depends how much processing is likely to be involved in producing deamjs outputs. If it's no more than a millisecond or two in the worst case, you might as well just keep it synchronous.

On the other hand if you get people wanting to generate MBs of test data from this, that could take a while to generate. Your current implementation will starve the event loop even though the API 'looks' asychronous. In this case you should definitely break up the work as suggested above.

Again, nice lib, and that's just my 2c!

[adleroliveira]

Thanks a lot for your comment.

You are totally right and I am already working on a version with true async resolution of the custom types (witch will bring a lot more power to the library).

I will be soon creating a brach with a draft of the new verion and you are more than welcome to check it out or contribute to it.

I appreciate your input.

Cheers

[ramikhalaf]

@yortus Wouldnt using process.nextTick() solve the undefined callback issue?

[yortus]

@ramikhalaf yes, but it wouldn't mitigate the starvation problem and dreamjs would still be effectively synchronous - see https://gist.github.com/brycebaril/ff86eeb90b53fd0c523e

[ramikhalaf]

@yortus I agree, but i meant it as its just a temporary work around that gives the callback async behavior until the new version.