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

Doctool may swallow errors due to unhandled async functions #30090

Closed
Fishrock123 opened this issue Oct 23, 2019 · 1 comment
Closed

Doctool may swallow errors due to unhandled async functions #30090

Fishrock123 opened this issue Oct 23, 2019 · 1 comment
Assignees
Labels
good first issue Issues that are suitable for first-time contributors. tools Issues and PRs related to the tools directory.

Comments

@Fishrock123
Copy link
Contributor

Basically, this code is not good:

fs.readFile(filename, 'utf8', async (er, input) => {
if (er) throw er;
const content = unified()
.use(markdown)
.use(html.preprocessText)
.use(json.jsonAPI, { filename })
.use(html.firstHeader)
.use(html.preprocessElements, { filename })
.use(html.buildToc, { filename, apilinks })
.use(remark2rehype, { allowDangerousHTML: true })
.use(raw)
.use(htmlStringify)
.processSync(input);
const basename = path.basename(filename, '.md');
const myHtml = await html.toHTML({ input, content, filename, nodeVersion });
const htmlTarget = path.join(outputDir, `${basename}.html`);
fs.writeFileSync(htmlTarget, myHtml);
const jsonTarget = path.join(outputDir, `${basename}.json`);
fs.writeFileSync(jsonTarget, JSON.stringify(content.json, null, 2));
});

Issues with that:

  • Async function set as a callback (unhandled)
  • sync I/O calls inside an async function
  • unified is synchronously invoked still

This is all the direct result of f4f856b, which is the result of something else. In hindsight, I wish I payed a bit more attention in my review of that.

Unfortunately my focus is elsewhere, so someone else should probably try to clean this up.

@Fishrock123 Fishrock123 added good first issue Issues that are suitable for first-time contributors. tools Issues and PRs related to the tools directory. labels Oct 23, 2019
@Purexo
Copy link

Purexo commented Oct 23, 2019

Hi, I'd like to solve this issue for participate at Node.js wich is a runtime i like a lot.
I've time tomorrow for rewrite this code.

I'll give you some news tomorrow on my advancement.

Maybe i'll commit with my pro account @tpoisseau

best regards.

manas2297 added a commit to manas2297/node that referenced this issue Oct 24, 2019
@targos targos closed this as completed in f035f55 Oct 27, 2019
targos pushed a commit that referenced this issue Oct 28, 2019
Use fs.promises for read and write file
Use unified().process wich is async instead processSync
html and json are write in parallel
errors are logged and exit process with `1` code

Fixes: #30090

PR-URL: #30106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this issue Nov 8, 2019
Use fs.promises for read and write file
Use unified().process wich is async instead processSync
html and json are write in parallel
errors are logged and exit process with `1` code

Fixes: #30090

PR-URL: #30106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this issue Nov 10, 2019
Use fs.promises for read and write file
Use unified().process wich is async instead processSync
html and json are write in parallel
errors are logged and exit process with `1` code

Fixes: #30090

PR-URL: #30106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this issue Nov 10, 2019
Use fs.promises for read and write file
Use unified().process wich is async instead processSync
html and json are write in parallel
errors are logged and exit process with `1` code

Fixes: #30090

PR-URL: #30106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this issue Nov 11, 2019
Use fs.promises for read and write file
Use unified().process wich is async instead processSync
html and json are write in parallel
errors are logged and exit process with `1` code

Fixes: #30090

PR-URL: #30106
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants