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

first version #39118

Closed
wants to merge 2 commits into from
Closed

first version #39118

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
420fef2
first version
daukadolt Jun 22, 2021
8ccef80
Merge branch 'nodejs:master' into handle-createAsyncIterator-prematur…
daukadolt Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/internal/streams/readable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ const {
ERR_INVALID_ARG_TYPE,
ERR_STREAM_PUSH_AFTER_EOF,
ERR_METHOD_NOT_IMPLEMENTED,
ERR_STREAM_UNSHIFT_AFTER_END_EVENT
ERR_STREAM_UNSHIFT_AFTER_END_EVENT,
ERR_STREAM_PREMATURE_CLOSE
} = require('internal/errors').codes;
const { validateObject } = require('internal/validators');

Expand Down Expand Up @@ -1123,6 +1124,10 @@ async function* createAsyncIterator(stream, options) {
next.call(this);
})
.on('close', function() {
if (endEmitted) {
throw new ERR_STREAM_PREMATURE_CLOSE();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing from an async event handler is not a good idea because it can't be caught (aside from the process-wide 'uncaughtException' event handler).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I almost forgot about that behavior in js.

Sorry, I am new to the project, so I am unsure about my solution, but what do you think about the following idea?

on('close', function() {
  if (endEmitted) {
    stream.emit('error', new ERR_STREAM_PREMATURE_CLOSE());
  } else {
    closeEmitted = true;
    next.call(this);
  }
});

I don't quite understand how, but my guess is that next.call(this) awakens the while loop that awaits a Promise:

else {
  await new Promise(next);
}

So my idea is, to emit error event with an instance of ERR_STREAM_PREMATURE_CLOSE so that it could, in turn, throw the error properly inside the try/catch block:

else if (errorEmitted) {
  throw error;
}

Why I am unsure:

  1. If I emit error event, when will it be processed? Will it be the next loop of the event loop?
  2. If so, can other events getting processed before the error event result in an unwanted behavior?

I believe that I also need an input from @ronag on this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to do anything in the event handler here. You should probably look at the finally block.

}

closeEmitted = true;
next.call(this);
});
Expand Down