-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
stream: don't wait for next item in take when finished #47132
stream: don't wait for next item in take when finished #47132
Conversation
Review requested:
|
.then(common.mustCall(() => { | ||
strictEqual(reached, false); | ||
})) | ||
.finally(() => resolve()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to do it. right?
the entire closure will be GCed...
FYI, I'm also trying to fix gonna be in another PR the need to wait for another item to abort a stream, this affect Edit: The error seems much larger and I don't have much time now to further debug it so opened an issue #47133 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This comment was marked as outdated.
This comment was marked as outdated.
Hey @debadree25, any idea why the test commit CI fail (the Jenkins one that is called when adding It's the second time now |
Seems to be flaky, I have restarted the build |
Thanks, what the difference between the test commit Jenkins job and the GitHub Actions one? Is there something I'm doing wrong? |
No no nothing wrong from your side at least doesn't seem like it, the Jenkins jobs basically run the tests in the other various platforms and configurations that nodejs needs to support but github actions dont yet, some of these maybe flaky or sometimes there maybe related failure which you can checkout by going on the link that the CI bot comments |
Commit Queue failed- Loading data for nodejs/node/pull/47132 ✔ Done loading data for nodejs/node/pull/47132 ----------------------------------- PR info ------------------------------------ Title stream: don't wait for next item in take when finished (#47132) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch rluvaton:dont-wait-for-next-item-even-when-finish -> nodejs:main Labels author ready, needs-ci Commits 3 - stream: dont wait for next item in take when finished - stream: update comment and lint - stream: trigger CI again Committers 1 - Raz Luvaton <16746759+rluvaton@users.noreply.github.com> PR-URL: https://github.com/nodejs/node/pull/47132 Reviewed-By: Robert Nagy Reviewed-By: Yagiz Nizipli Reviewed-By: Erick Wendel Reviewed-By: Matteo Collina Reviewed-By: Debadree Chatterjee ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47132 Reviewed-By: Robert Nagy Reviewed-By: Yagiz Nizipli Reviewed-By: Erick Wendel Reviewed-By: Matteo Collina Reviewed-By: Debadree Chatterjee -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 17 Mar 2023 09:56:39 GMT ✔ Approvals: 5 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/47132#pullrequestreview-1345627091 ✔ - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/47132#pullrequestreview-1345903804 ✔ - Erick Wendel (@erickwendel): https://github.com/nodejs/node/pull/47132#pullrequestreview-1346006254 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/47132#pullrequestreview-1346652710 ✔ - Debadree Chatterjee (@debadree25): https://github.com/nodejs/node/pull/47132#pullrequestreview-1347525558 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-03-19T16:00:00Z: https://ci.nodejs.org/job/node-test-pull-request/50470/ - Querying data for job/node-test-pull-request/50470/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 47132 From https://github.com/nodejs/node * branch refs/pull/47132/merge -> FETCH_HEAD ✔ Fetched commits as fbd526b15a0b..f233a6dff4d6 -------------------------------------------------------------------------------- [main 0c47595d4f] stream: dont wait for next item in take when finished Author: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Fri Mar 17 11:53:51 2023 +0200 2 files changed, 27 insertions(+), 2 deletions(-) [main 74da41c71b] stream: update comment and lint Author: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Fri Mar 17 11:57:45 2023 +0200 1 file changed, 3 insertions(+), 3 deletions(-) The previous cherry-pick is now empty, possibly due to conflict resolution. If you wish to commit it anyway, use:https://github.com/nodejs/node/actions/runs/4461834999 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@debadree25 any update? Is this happening only for my PR or for all node PRs? 😬😬 |
I think this might be happening for all PRs but I am not sure nonetheless I am requesting fresh ci again let's hope it succeeds so sorry for the delay but better to be safe and correct 😅 |
This comment was marked as outdated.
This comment was marked as outdated.
Landed in 22537f3 |
PR-URL: #47132 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Erick Wendel <erick.workspace@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
PR-URL: #47132 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Erick Wendel <erick.workspace@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
PR-URL: #47132 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Erick Wendel <erick.workspace@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
currently, in the
take
operator, we wait for the next item in the stream to check whether to finish or notnow we don't need to wait anymore