Skip to content
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

events: timing issue awaiting multiple events emitted in same nextTick #32431

Closed
jasnell opened this issue Mar 22, 2020 · 5 comments
Closed

events: timing issue awaiting multiple events emitted in same nextTick #32431

jasnell opened this issue Mar 22, 2020 · 5 comments
Labels
doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. promises Issues and PRs related to ECMAScript promises.

Comments

@jasnell
Copy link
Member

jasnell commented Mar 22, 2020

Just leaving this here as an issue worth investigating. Not sure there's anything we can do about it other than documenting it...

Multiple events emitted on the same process.nextTick() may be missed when using await once(). For example, when running the following, both the 'bar' and 'foo' events are emitted but the async function foo() never completes because await once(myEE, 'foo') occurs after the foo event is actually emitted.

'use strict';

const { EventEmitter, once } = require('events');

const myEE = new EventEmitter();

async function foo() {
  await once(myEE, 'bar');
  console.log('bar');

  await once(myEE, 'foo');
  console.log('foo');
}

process.nextTick(() => {
  myEE.emit('bar');
  myEE.emit('foo');
});

foo().then(() => console.log('done'));

setTimeout(() => {}, 1000);

The only way to catch both events is to use Promise.all() or Promise.allSettled(), or to not use await with once(myEE, 'bar')

'use strict';

const { EventEmitter, once } = require('events');

const myEE = new EventEmitter();

async function foo() {
  await once(myEE, 'bar');
  console.log('bar');

  await once(myEE, 'foo');
  console.log('foo');
}

async function foo2() {
  await Promise.all([once(myEE, 'bar'), once(myEE, 'foo')]);
  console.log('foo', 'bar');
}

process.nextTick(() => {
  myEE.emit('bar');
  myEE.emit('foo');
});

foo2().then(() => console.log('done'));

setTimeout(() => {}, 1000);

Why does this happen? It is because the process.nextTick is processed before the microtask queue. Both events are emitted synchronously before the microtask queue is processed. The await once(myEE, 'bar') does not continue until the microtask queue is executed, after the 'foo' event is emitted, so await once(myEE, 'foo') ends up waiting forever, having missed the actual event.

@jasnell jasnell added events Issues and PRs related to the events subsystem / EventEmitter. promises Issues and PRs related to ECMAScript promises. doc Issues and PRs related to the documentations. labels Mar 22, 2020
@mcollina
Copy link
Member

I agree that this looks like a problem to investigate. I don’t think we have a solution, or a solution is possible without a perf cost.

@addaleax
Copy link
Member

I don’t think there’s anything we can do about this besides maybe documenting this – I would say it’s expected behaviour that can still be surprising sometimes.

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2020

Yeah, there's really nothing that can be done without a major refactor or a major perf hit. I think documenting is the only way to go

@hassaanp
Copy link
Contributor

Hi,

I was wondering where in the docs should this go. If someone can point me to the appropriate section, I'd be happy to log this behavior.

@jasnell
Copy link
Member Author

jasnell commented Jun 17, 2020

@hassaanp ... my apologies for not seeing your comment earlier. This should likely best be added as a subsection to the documentation for the once function in the doc/api/events.md file.

Note that the same issue arises when using the on() function in a for await loop. The only solution there is that any event handlers for other events you may be interested in must be attached before entering the for await loop or they will end up being lost.

jasnell added a commit to jasnell/node that referenced this issue Jul 9, 2020
@jasnell jasnell closed this as completed in 62bdb7e Jul 9, 2020
MylesBorins pushed a commit that referenced this issue Jul 14, 2020
Fixes: #32431

PR-URL: #34220
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 16, 2020
Fixes: #32431

PR-URL: #34220
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
codebytere pushed a commit that referenced this issue Sep 27, 2020
Fixes: #32431

PR-URL: #34220
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
addaleax pushed a commit that referenced this issue Sep 28, 2020
Fixes: #32431

PR-URL: #34220
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants