asciidoctor / asciidoctor-cli.js

The Command Line Interface (CLI) for Asciidoctor.js
https://asciidoctor.org
MIT License
15 stars 9 forks source link

Invoker does not wait for all output to be written to stdout #196

Closed mojavelinux closed 1 year ago

mojavelinux commented 2 years ago

If the CLI is used to pipe the result to stdout, the Invoker is not waiting for the process to finish writing before calling exit. This situation occurs when the amount of output exceeds the write limit for a single tick. In other words, the write is not fully synchronous.

Here's a patch to convertFromStdin that will fix the problem when piping from stdin to stdout:

if (options.to_file === Opal.gvars.stdout) {
  await once(process.stdout.end(), 'close')
}

However, I think this will be needed wherever to_file is stdout.

In order to reproduce this problem, the asciidoctorjs bin script needs to be called via spawn (or similar). If the test pipes a large file to the command, it will not receive the whole converted file back to stdout without this patch.

zkaip commented 2 years ago

@mojavelinux

UnhandledPromiseRejectionWarning: ReferenceError: once is not defined

'once' function is not found

zkaip commented 2 years ago
  static async convertFromStdin (options, args) {
    const data = await Invoker.readFromStdin()
    if (args.timings) {
      const timings = asciidoctor.Timings.create()
      const instanceOptions = Object.assign({}, options, { timings })
      Invoker.convert(asciidoctor.convert, data, instanceOptions)
      timings.printReport(process.stderr, '-')
    } else {
      Invoker.convert(asciidoctor.convert, data, options)
    }
    if (options.to_file === Opal.gvars.stdout) {
      await once(process.stdout.end(), 'close')
    }
  }
mojavelinux commented 2 years ago

'once' function is not found

You need to require it:

const { once } = require('events')
zkaip commented 2 years ago

Yes, I resolved.

Will the patch be updated to this project?

mojavelinux commented 2 years ago

If someone submits a PR, then likely yes. That PR must include two things:

1) The change will be need to be applied wherever to_file is stdout. 2) There must be at least one test to verify the behavior, and ideally one for each code path

mojavelinux commented 1 year ago

Thanks for following through with the fix, Guillaume!