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.

Sign up for GitHub

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

Jump to bottom

test: async iterate destroyed stream #28995

Closed
wants to merge 1 commit into from
Closed

test: async iterate destroyed stream #28995

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 6, 2019 •
edited
Loading

Currently we will deadlock on destroyed streams...

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
Copy link
Member Author

ronag commented Aug 6, 2019 •
edited
Loading

@mcollina: Should this maybe result in a ERR_STREAM_DESTROYED?

@ronag ronag mentioned this pull request Aug 6, 2019
Closed
2 tasks
@mcollina
Copy link
Member

mcollina commented Aug 6, 2019

Would you mind adding a unit test?

@mcollina mcollina added lts-watch-v10.x stream Issues and PRs related to the stream subsystem. labels Aug 6, 2019
@ronag ronag mentioned this pull request Aug 6, 2019
Closed
7 tasks
@ronag
Copy link
Member Author

ronag commented Aug 6, 2019

This would be fixed by and is blocked by #28748

@ronag
Copy link
Member Author

ronag commented Aug 6, 2019

Added test.

@ronag ronag force-pushed the stream-async-iterator-destroyed branch 3 times, most recently from 4a18e90 to 39594ee Compare August 6, 2019 11:04
@trivikr
Copy link
Member

trivikr commented Aug 6, 2019

Should subsystem be test instead of stream?

@ronag
Copy link
Member Author

ronag commented Aug 6, 2019

@trivikr: Let's see what happens with #28748. If it is merged and fixes this issue, then yes, it should be test.

@ronag
Copy link
Member Author

ronag commented Aug 24, 2019

@Trott: blocked label?

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Aug 24, 2019
@ronag ronag changed the title stream: async iterate destroyed stream test: async iterate destroyed stream Sep 23, 2019
@ronag ronag force-pushed the stream-async-iterator-destroyed branch from 39594ee to 029fe89 Compare September 23, 2019 07:39
@ronag
Copy link
Member Author

ronag commented Sep 23, 2019 •
edited
Loading

renamed into "test" scope

@ronag ronag force-pushed the stream-async-iterator-destroyed branch 3 times, most recently from bed5ced to 0d6ba10 Compare September 24, 2019 21:06
@ronag ronag force-pushed the stream-async-iterator-destroyed branch from 0d6ba10 to 87c5750 Compare September 24, 2019 21:06
@ronag
Copy link
Member Author

ronag commented Sep 25, 2019

@Trott: this is no longer blocked

@trivikr trivikr removed the blocked PRs that are blocked by other issues or PRs. label Sep 26, 2019
@ronag
Copy link
Member Author

ronag commented Nov 11, 2019

@Trott: This is blocked again (#29724) since the fix was reverted.

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Nov 12, 2019
@ronag ronag added dont-land-on-v10.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. lts-watch-v10.x labels Feb 14, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29119/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29123/

ronag added a commit that referenced this pull request Feb 14, 2020
@ronag
test: async iterate destroyed stream
df1592d 
PR-URL: #28995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@ronag
Copy link
Member Author

ronag commented Feb 14, 2020

Landed in df1592d

@ronag ronag closed this Feb 14, 2020
@BethGriggs BethGriggs mentioned this pull request Apr 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ronag @mcollina @trivikr @nodejs-github-bot @jasnell @addaleax @Trott @BridgeAR