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.

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

docs: writable streams from async iterators example issue #31222

Closed
ronag opened this issue Jan 6, 2020 · 6 comments
Closed

docs: writable streams from async iterators example issue #31222

ronag opened this issue Jan 6, 2020 · 6 comments

Comments

@ronag
Copy link
Member

ronag commented Jan 6, 2020

Given the example from https://nodejs.org/api/stream.html#stream_piping_to_writable_streams_from_async_iterators.

There is a problem where this can cause an unhandled exception if the writable emits an 'error' before the first chunk arrives from the iterator, e.g. if writable is a fs stream and it fails while opening the file.

  const writable = fs.createWriteStream('./file');

  for await (const chunk of iterator) {
    // Handle backpressure on write().
    if (!writable.write(chunk))
      await once(writable, 'drain');
  }
  writable.end();
  // Ensure completion without errors.
  await finished(writable);
@ronag
Copy link
Member Author

ronag commented Jan 6, 2020

@mcollina / @nodejs/streams: I can't think of a elegant way to resolve this. Any ideas?

Otherwise, I would suggest we remove this example until we find a better way in order to avoid people using this pattern and thinking it's safe.

@ronag
Copy link
Member Author

ronag commented Jan 6, 2020

Maybe?

EDIT: Or rather maybe not :D

  const writable = fs.createWriteStream('./file');

  // catch 'error' while not writing
  // EDIT: add a noop .catch to avoid unhandledRejection before awaiting.
  const finishedPromise = finished(writable).catch(() => {}); 
  for await (const chunk of iterator) {
    if (writable.destroyed) // assumes destroy() on 'error'
      break;

    // Handle backpressure on write().
    if (!writable.write(chunk))
      await once(writable, 'drain');
  }
  writable.end();
  // Ensure completion without errors.
  await finishedPromise;

@himself65
Copy link
Member

what is the const finishedPromise = finished(writable); meaning? 🤔

@ronag
Copy link
Member Author

ronag commented Jan 6, 2020

what is the const finishedPromise = finished(writable); meaning? 🤔

it returns a promise that will resolve or reject when the writable finishes or errors.

Actually, that won't work either since we currently do unhandled rejection for rejected promises without a catch.

@ronag
Copy link
Member Author

ronag commented Jan 6, 2020

@himself65: "Fixed". It's not good though. Maybe not that one.

@ronag
Copy link
Member Author

ronag commented Jan 6, 2020

Another try. What if we use some kind of event that signals that no error can occur until a write is issued?

  const writable = fs.createWriteStream('./file');

  await once(writable, 'ready');
  // No error possible until writable.write

  for await (const chunk of iterator) {
    // Handle backpressure on write().
    if (!writable.write(chunk))
      await once(writable, 'drain');
  }
  writable.end();
  // Ensure completion without errors.
  await finished(writable).catch(() => {}); 

Though unfortunately, that does not exist currently. Probably also not possible to enforce since one can call .destroy(err) at any time. I don't like this either...

ronag added a commit to nxtedition/node that referenced this issue Jan 7, 2020
The for await loop into writable loop could cause an unhandled exception
in the case where we are waiting for data from the async iterable and
this no `'error'` handler is registered on the writable.

Fixes: nodejs#31222
@Trott Trott closed this as completed in 1b8eb22 Jan 10, 2020
Trott pushed a commit that referenced this issue Jan 10, 2020
PR-URL: #31252
Fixes: #31222
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 16, 2020
The for await loop into writable loop could cause an unhandled exception
in the case where we are waiting for data from the async iterable and
this no `'error'` handler is registered on the writable.

Fixes: #31222

PR-URL: #31252
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 16, 2020
PR-URL: #31252
Fixes: #31222
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Mar 14, 2020
The for await loop into writable loop could cause an unhandled exception
in the case where we are waiting for data from the async iterable and
this no `'error'` handler is registered on the writable.

Fixes: #31222

PR-URL: #31252
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Mar 14, 2020
PR-URL: #31252
Fixes: #31222
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Mar 17, 2020
The for await loop into writable loop could cause an unhandled exception
in the case where we are waiting for data from the async iterable and
this no `'error'` handler is registered on the writable.

Fixes: #31222

PR-URL: #31252
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Mar 17, 2020
PR-URL: #31252
Fixes: #31222
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 a pull request may close this issue.

2 participants