Skip to content

fix: tolerate more from async_hooks#1275

Merged
erights merged 4 commits intomasterfrom
markm-async-hooks-again-ohmy
Aug 29, 2022
Merged

fix: tolerate more from async_hooks#1275
erights merged 4 commits intomasterfrom
markm-async-hooks-again-ohmy

Conversation

@erights
Copy link
Copy Markdown
Contributor

@erights erights commented Aug 28, 2022

Our attempt to safely tolerate Node async_hooks was safe, but missed a case we could handle safely and which is provoked at least by the vscode debugger.

Node's async_hooks currently contains the following code, which we can also safely tolerate

function destroyTracking(promise, parent) {
  trackPromise(promise, parent);
  const asyncId = promise[async_id_symbol];
  const destroyed = { destroyed: false };
  promise[destroyedSymbol] = destroyed;
  registerDestroyHook(promise, asyncId, destroyed);
}

However, before this PR we did not know to expect this. I just encountered this failure when trying to debug some of our code in the vscode debugger. The additional record that Node adds here is one we can tolerate safely. This PR's expectations includes this additional record, so that we can safely tolerate it in environments, like the vscode debugger, that provoke it.

Reviews: How do I test this? I have already tried it experimentally on the debugger experience that was blocking me. This code got me past that problem.

Copy link
Copy Markdown
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is safe and can be merged as is, my review is mostly stylistic nits.

Sorry for not writing the correct check in the first place. Could you please open issue for me to look into unit testing this somehow?

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Aug 29, 2022

Could you please open issue for me to look into unit testing this somehow?

#1276

@erights erights merged commit 1a0b123 into master Aug 29, 2022
@erights erights deleted the markm-async-hooks-again-ohmy branch August 29, 2022 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants