Skip to content

Conversation

@ronag
Copy link
Member

@ronag ronag commented Jul 23, 2019

finished should not leave dangling listeners and leave the stream in the same state after as before.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag ronag force-pushed the fix-eos-cleanup branch 2 times, most recently from 157348b to e66269c Compare July 24, 2019 09:51
Heap dumps can be taken either through the inspector or the public API
for it during an async_hooks init() hook, but at that point the
AsyncWrap in question is not done initializing yet and virtual methods
cannot be called on it.

Address this issue (somewhat hackily) by excluding `AsyncWrap`
instances which have not yet executed their `init()` hook fully
from heap dumps.

Fixes: nodejs#28786

PR-URL: nodejs#28789
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ronag ronag force-pushed the fix-eos-cleanup branch 2 times, most recently from ce88059 to 77bfb20 Compare July 24, 2019 09:54
@Fishrock123 Fishrock123 added the stream Issues and PRs related to the stream subsystem. label Jul 24, 2019
@Trott
Copy link
Member

Trott commented Jul 25, 2019

Even though the GitHub interface isn't complaining about it, I think this might need a rebase? This doesn't apply cleanly for me on master, and the diff seems to include stuff that is already in another commit authored by @addaleax that has already landed. (I feel like there must be something I'm doing wrong. None of what I'm writing here makes sense.)

@mscdex
Copy link
Contributor

mscdex commented Jul 26, 2019

Perhaps another commit was accidentally amended with the changes from this PR?

@addaleax
Copy link
Member

@Trott Using 3-way git am/git cherry-pick should resolve that, and so the changes would end up being removed anyway, whether it’s through a rebase here or when landing the PR.

@Trott
Copy link
Member

Trott commented Jul 26, 2019

@Trott Using 3-way git am/git cherry-pick should resolve that, and so the changes would end up being removed anyway, whether it’s through a rebase here or when landing the PR.

Oh, of course, git am doesn't add the -3 automatically when it fails without it, the way git cherry-pick does (at least for me) so yeah, my goof. Thanks!

@ronag
Copy link
Member Author

ronag commented Jul 26, 2019

This is blocked by #28818

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Jul 26, 2019
@ronag ronag mentioned this pull request Aug 2, 2019
4 tasks
@ronag
Copy link
Member Author

ronag commented Aug 2, 2019

If the user wants cleanup they can just call the callback.

e.g.

const cleanup = finished(...streams, err => {
  cleanup();
  // ...
});

@ronag ronag closed this Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked PRs that are blocked by other issues or PRs. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants