abbr / deasync

Turns async function into sync via JavaScript wrapper of Node event loop
MIT License
964 stars 73 forks source link

deasync hangs in an asynchronous context #21

Open sterling opened 9 years ago

sterling commented 9 years ago

When using a deasynced function in asynchronous code, deasync hangs and never returns.

var deasync = require('deasync');

function async(cb) {
  setTimeout(function() {
    cb(null, 'value');
  }, 0);
}

function sync() {
  return deasync(async)();
}

console.log('start', sync());
async(function(err, val) {
  console.log('async result', val);
  console.log(sync());
  console.log('done');
});

Notice that when run, the second call to sync hangs. I was originally using promises when testing this and derived the previous code from this:

var Promise = require('q');
var deasync = require('deasync');

function async() {
  return Promise.resolve('value');
}

function sync() {
  var result, done;
  async().then(function(response) {
    result = response;
  }).finally(function() {
    done = true;
  });

  deasync.loopWhile(function() { return !done; });

  return result;
}

console.log('start', sync());
async().then(function(result) {
  console.log('async result', result);
  return sync();
}).then(function(value) {
  console.log(value);
  console.log('done');
});

I also tried the above example using the Bluebird promise library and ended up with the same results. In the promise example, adding a console.log inside the loopWhile handler shows that it is stuck checking done since the promise chain never completes.

maxvyaznikov commented 9 years ago

Much more simple example:

var deasync = require('deasync');

function async(cb) {
    console.log('1');
    setTimeout(function() {
        console.log('2');
        cb(null, 'value');
        console.log('3');
    }, 0);
}

console.log('A', deasync(async)());
setTimeout(function() {
    console.log('B', deasync(async)());
}, 0);

I had:

$ node test.js 
1
2
3
A value
1
^C
abbr commented 9 years ago

Although haven't found a fix yet, the problem appears only if the nested setTimeout having same timeout parameter. In @maxvyaznikov 's example, changing one of the timeout param away from 0 and 1 (b/c 0 is treated as 1 by Node.js implementation of setTimeout) will avoid hung. This serves as a temporary workaround.

marcellodesales commented 9 years ago

Functions with Bluebird Promises get stuck

I have a bluebird promises implementation of a function that returns the CPS function that hangs as well... https://github.com/marcellodesales/node-pom-parser/blob/master/lib/index.js#L36-L59

If I use the following code, it will just hang:

/**
 * Setup the temporary pom.xml settings as we still use it.
 * TODO: Remove this when we have a registry
 */
Core.prototype._loadPomXmlSettings = function _loadPomXmlSettings() {
  this.APP_POM_PATH = this.APP_DIR + "/pom.xml";

  if (fs.existsSync(this.APP_POM_PATH)) {
    var parser = deasync(pomParser.parse);
    console.log(parser);

    var resp = parser({filePath: this.APP_POM_PATH});
    console.log(resp);
    this.pom = resp.pomObject;
  }
};

This only prints the following:

[Function]

When it should print the object below the resp call.

Functions without it does not

I have implemented others functions using the same CPS pattern without bluebird and it works as expected... Those below work find!!!

https://github.com/marcellodesales/node-newrelic-api/blob/master/src/deployments.js#L90

https://github.com/marcellodesales/node-newrelic-api/blob/master/src/applications.js#L46

@abbr any suggestion for this one?

abbr commented 9 years ago

The problem appears to be a Node bug. Have filed issue #25831 and pr #2830

marcellodesales commented 9 years ago

@abbr thanks for following up...

kjhangiani commented 9 years ago

Ran into this issue as well, using node 0.12.7, deasync hangs if called within a .then block of an async function. I noticed a PR has been submitted, @abbr, do you know if this has been merged in yet or if there is any way to port the fix?

My specific use case is within a .then block of a sequelize.findAll query (sequelize uses bluebird for promises iirc)

sterling commented 8 years ago

@abbr has there been any progress on this?

abbr commented 8 years ago

No. If the issue affects a run time env under your control and there is no workaround, you can compile NodeJS from source with my pr applied (move 1 line of code in lib/timers.js)

Aminadav commented 8 years ago

@sterling , @maxvyaznikov The code your provide to reproduce the bug, will work, in my merge.

dy commented 8 years ago

@abbr could you please react on the PR #52?

I faced that issue in glslify-sync, it is quite acute:

setTimeout(function () {
 var source = glslify('./file.glsl');
});

Just hangs. Setting timeout to 0, 1 does not help. Please, help. Oh, I see, #52 is not very good.

@abbr you have mentioned this for https://github.com/nodejs/node-v0.x-archive/issues/25831, but is there the same issue for node 4.x, 5.x? because I face this in node 5.5.0

vellotis commented 8 years ago

It's a pity.. Seems that it is thought to be a nofix by node team @ nodejs/node#2830.

clayrisser commented 5 years ago

Is there any way to work around this bug?

leo60228 commented 5 years ago

I'd say this package isn't really necessary more, now that async functions exist.

On Thu, Jan 24, 2019, 7:31 AM Jam Risser notifications@github.com wrote:

Is there any way to work around this bug?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/abbr/deasync/issues/21#issuecomment-457180359, or mute the thread https://github.com/notifications/unsubscribe-auth/AH996cZP3D8NQJAhCQK0wR_E4gyPdNIsks5vGaeLgaJpZM4FT0W0 .

clayrisser commented 5 years ago

Not true. I need it to run some async code on import.

jardicc commented 5 years ago

I'd say this package isn't really necessary more, now that async functions exist. On Thu, Jan 24, 2019, 7:31 AM Jam Risser @.***> wrote: Is there any way to work around this bug? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#21 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AH996cZP3D8NQJAhCQK0wR_E4gyPdNIsks5vGaeLgaJpZM4FT0W0 .

"deasync" is thread blocking meanwhile async function will split thread in two parts so you need to wrap all code in async/await or into then(). So you might end up with refactoring of all your code if your function you want to deasync is deeply nested.

jardicc commented 5 years ago

I tried to use socket.setTimout(). I created a server using named pipes and I replaced global.setTimeout() with it because you can set milliseconds and callback. I could combine original setTimeout with custom made timeout with same ms value but it failed when I used 2× my same custom timeout.

I also tried to use named pipes to execute timeout in different separated process where is not deasync and it failed which is strange because it wasn't caused by native setTimeout? So we should verify if setTimeout is the real bug reason.

I also tried to use vm.runInNewContext(code[, sandbox[, options]]) and it also failed because of there no setTimeout in global context so you have to pass it in.

hyperupcall commented 4 years ago

@codejamninja

Is there any way to work around this bug?

i was able to get around this by not using deasync and hiding my asyncronous code behind the syncronous require call - i put the needed information (from the async function call) on the global context. there's probably a better way, but that's how i did it

lmcsu commented 2 years ago

Wrapping the code into process.nextTick somehow helped me though, I don't know if this is related