stream: allow stream to stay open after take #47023

rluvaton · 2023-03-09T06:36:56Z

fix #46980

TODO

add tests
add docs

fix nodejs#46980

nodejs-github-bot · 2023-03-09T06:37:00Z

Review requested:

@nodejs/streams

ronag · 2023-03-09T07:51:23Z

lib/internal/streams/operators.js

+  if (options?.destroyStream != null) {
+    validateBoolean(options.destroyStream, 'options.destroyStream');
+  }


Maybe another name would be better? Not sure what though... @benjamingr

rluvaton · 2023-03-09T09:18:14Z

lib/internal/streams/operators.js

+    for await (const val of this.iterator({ destroyOnReturn: options?.destroyStream ?? true })) {
      if (options?.signal?.aborted) {
        throw new AbortError();
      }


I think that if the stream fails we should close the stream, WDYT @ronag ?

@mcollina any thoughts on your side?

after thinking I think it should be closed on an error as in the iterator helpers proposal spec the underlying iterator should be closed when it failed

mcollina

Thanks for opening a PR! Can you please add a unit test?

mcollina · 2023-03-09T10:48:53Z

Docs are also missing

rluvaton · 2023-03-09T22:16:52Z

Hey @mcollina I've added tests and docs :)

mcollina

lgtm

rluvaton · 2023-03-10T10:39:25Z

@mcollina is something holding this back from merging? Or we waiting for the TC39?

mcollina · 2023-03-10T12:27:46Z

we have 48 hours grace period to allow for folks across the globe to review

github-actions · 2023-03-10T12:44:45Z

Failed to start CI

- Validating Jenkins credentials
✔  Jenkins credentials valid
- Starting PR CI job
✘  Failed to start PR CI: 403 Forbidden

https://github.com/nodejs/node/actions/runs/4384604326

github-actions · 2023-03-10T13:50:34Z

Failed to start CI

- Validating Jenkins credentials
✔  Jenkins credentials valid
- Starting PR CI job
✘  Failed to start PR CI: 403 Forbidden

https://github.com/nodejs/node/actions/runs/4385139704

nodejs-github-bot · 2023-03-11T19:26:13Z

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

doc/api/stream.md

Co-authored-by: Debadree Chatterjee <debadree333@gmail.com>

doc/api/stream.md

…fter-take-46980' into feat/allow_stream-to-stay-open-after-take-46980

debadree25

I think you would need to remove the merge commit since supposedly it can cause issues with the tooling everything else is good!!

rluvaton · 2023-03-12T10:09:10Z

I think you would need to remove the merge commit since supposedly it can cause issues with the tooling everything else is good!!

aren't we using squash and merge?

debadree25 · 2023-03-12T10:13:27Z

I think you would need to remove the merge commit since supposedly it can cause issues with the tooling everything else is good!!

aren't we using squash and merge?

Ref: #46910 (comment)

benjamingr

I don't think discussion has exhausted itself in the spec issue and I don't want to deviate from it until we proved we must

debadree25 · 2023-03-12T12:46:10Z

I don't think discussion has exhausted itself in the spec issue and I don't want to deviate from it until we proved we must

Should be add the blocked label for now then?

benjamingr

Yeah sorry I meant to explicitly block and hit the wrong button

michaelficarra

I think what you really want is a chunk (which will be proposed for inclusion in standard iterators in the future) or a nextN helper like

function nextN(iterator, n) {
  const result = [];
  const next = iterator.next;
  for (; n > 0; --n) {
    cont { done, value } = next.call(iterator);
    if (done) break;
    result.push(value);
  }
  return result;
}

rluvaton · 2023-03-15T21:42:41Z

@michaelficarra no problem but maybe could you update the proposal first so we won't have this back and forth

FYI I think nextN is better name than chunk

michaelficarra · 2023-03-15T21:46:46Z

@rluvaton chunk is a different method than the nextN that I wrote. See tc39/proposal-iterator-helpers#71 (comment) or https://www.npmjs.com/package/chunk.

rluvaton · 2023-03-16T19:51:21Z

@rluvaton chunk is a different method than the nextN that I wrote. See tc39/proposal-iterator-helpers#71 (comment) or npmjs.com/package/chunk.

@michaelficarra

Oh, after looking at chunk what I really want is nextN which is basically a non-closing take

Do you think that non-closing take will be added?

chunk is less what I want as:

I don't want chunks I just want the x items and the rest left unchanged (like in the headers row from CSV file - explained more in the linked issue)
The data is not evenly distributed
It won't let me get X items without closing the stream, I would just get another iterator that emits in chunks which I can't exit as it will close

rluvaton · 2023-03-16T20:18:09Z

Having nextN as an operator that returns iterable would enable:

better chaining
better efficiency (no need to save the values in case we don't need - without .toArray())

Example of usages:

having it as an operator:

const responses = await topVisitedUrlsIterator
	.filter(noEmptyLine)
	.drop(1) // Header
	.nextN(8)
	.map(parseLine)
	.map(value => apiCall(value), { concurrency: 4 })
	.toArray();

having it as a static function:

const values = await nextN(
  topVisitedUrlsIterator
    .filter(noEmptyLine)
    .drop(1),
  8)
  .map(parseLine);

// How would I use the concurrency that we would have in the map?
const responses = concurrentMap(values, (value) => apiCall(value), 4);

# Conflicts: # lib/internal/streams/operators.js

nodejs-github-bot · 2023-08-10T09:25:41Z

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

rluvaton added 2 commits March 9, 2023 00:34

stream: allow stream to stay open after take

ff564fd

fix nodejs#46980

stream: close stream on error

abce0a7

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 9, 2023

rluvaton mentioned this pull request Mar 9, 2023

stream: Add option to Readable.take operator to not close the stream #46980

Closed

rluvaton added 2 commits March 9, 2023 08:41

stream: remove unnecessary code

df52ac1

stream: add import

2f38460

ronag approved these changes Mar 9, 2023

View reviewed changes

ronag reviewed Mar 9, 2023

View reviewed changes

stream: fix when options is missing

d52c8ab

rluvaton commented Mar 9, 2023

View reviewed changes

mcollina requested changes Mar 9, 2023

View reviewed changes

rluvaton added 7 commits March 9, 2023 14:58

stream: added tests

c8a4849

stream: fail in stream will close the stream

8af907f

stream: fix taking item from next iteration

444cf40

stream: format

6e09304

stream: use destructure

2542401

stream: trigger ci

1ae2061

stream: add docs

d3fc3c6

mcollina approved these changes Mar 10, 2023

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2023

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2023

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Mar 10, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2023

debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 11, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 11, 2023

debadree25 reviewed Mar 12, 2023

View reviewed changes

doc/api/stream.md Outdated Show resolved Hide resolved

Update doc/api/stream.md

55907e6

Co-authored-by: Debadree Chatterjee <debadree333@gmail.com>

debadree25 reviewed Mar 12, 2023

View reviewed changes

doc/api/stream.md Outdated Show resolved Hide resolved

rluvaton added 2 commits March 12, 2023 11:13

stream: update docs

99322a8

Merge remote-tracking branch 'origin/feat/allow_stream-to-stay-open-a…

3fbdf40

…fter-take-46980' into feat/allow_stream-to-stay-open-after-take-46980

debadree25 approved these changes Mar 12, 2023

View reviewed changes

stream: update docs

152e60b

benjamingr reviewed Mar 12, 2023

View reviewed changes

ronag added the blocked PRs that are blocked by other issues or PRs. label Mar 12, 2023

benjamingr requested changes Mar 12, 2023

View reviewed changes

michaelficarra reviewed Mar 15, 2023

View reviewed changes

rluvaton added 2 commits May 6, 2023 23:25

Merge branch 'main' into feat/allow_stream-to-stay-open-after-take-46980

1b8fa15

# Conflicts: # lib/internal/streams/operators.js

fix conflict

4ea9d73

rluvaton mentioned this pull request May 20, 2023

stream: unref #48007

Open

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Aug 8, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: allow stream to stay open after take #47023

stream: allow stream to stay open after take #47023

rluvaton commented Mar 9, 2023 •

edited

Loading

nodejs-github-bot commented Mar 9, 2023

ronag Mar 9, 2023

rluvaton Mar 9, 2023

rluvaton Mar 9, 2023

rluvaton Mar 9, 2023

mcollina left a comment

mcollina commented Mar 9, 2023

rluvaton commented Mar 9, 2023

mcollina left a comment

rluvaton commented Mar 10, 2023

mcollina commented Mar 10, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 10, 2023

nodejs-github-bot commented Mar 11, 2023

debadree25 left a comment •

edited

Loading

rluvaton commented Mar 12, 2023

debadree25 commented Mar 12, 2023

benjamingr left a comment

debadree25 commented Mar 12, 2023

benjamingr left a comment

michaelficarra left a comment

rluvaton commented Mar 15, 2023

michaelficarra commented Mar 15, 2023

rluvaton commented Mar 16, 2023 •

edited

Loading

rluvaton commented Mar 16, 2023 •

edited

Loading

nodejs-github-bot commented Aug 10, 2023

stream: allow stream to stay open after take #47023

Are you sure you want to change the base?

stream: allow stream to stay open after take #47023

Conversation

rluvaton commented Mar 9, 2023 • edited Loading

TODO

nodejs-github-bot commented Mar 9, 2023

ronag Mar 9, 2023

Choose a reason for hiding this comment

rluvaton Mar 9, 2023

Choose a reason for hiding this comment

rluvaton Mar 9, 2023

Choose a reason for hiding this comment

rluvaton Mar 9, 2023

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Mar 9, 2023

rluvaton commented Mar 9, 2023

mcollina left a comment

Choose a reason for hiding this comment

rluvaton commented Mar 10, 2023

mcollina commented Mar 10, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 10, 2023

nodejs-github-bot commented Mar 11, 2023

debadree25 left a comment • edited Loading

Choose a reason for hiding this comment

rluvaton commented Mar 12, 2023

debadree25 commented Mar 12, 2023

benjamingr left a comment

Choose a reason for hiding this comment

debadree25 commented Mar 12, 2023

benjamingr left a comment

Choose a reason for hiding this comment

michaelficarra left a comment

Choose a reason for hiding this comment

rluvaton commented Mar 15, 2023

michaelficarra commented Mar 15, 2023

rluvaton commented Mar 16, 2023 • edited Loading

rluvaton commented Mar 16, 2023 • edited Loading

nodejs-github-bot commented Aug 10, 2023

rluvaton commented Mar 9, 2023 •

edited

Loading

debadree25 left a comment •

edited

Loading

rluvaton commented Mar 16, 2023 •

edited

Loading

rluvaton commented Mar 16, 2023 •

edited

Loading