From c9f6a0f50c3be58e12b4bac791abc61d1e7b93e6 Mon Sep 17 00:00:00 2001 From: Konstantin Vasserman Date: Wed, 18 Aug 2021 15:13:49 -0400 Subject: [PATCH 1/2] child process exit event fires before IO is complete, causing errors Under heavy load (in the context of web application, for example using nestjs) some of the requests fail: 1. According to child process documentation (https://nodejs.org/docs/latest-v14.x/api/child_process.html#child_process_event_exit), exit even can fire before stdio is closed. In such cases respond() is being called with code 0 (success), no error and no data. This results in an error in the calling code, because filename is expected but is not returned. I added additional error checking to respond() to handle this case. 2. Given the above, it's probably not a good idea to subscribe to exit event at all. I commented out the on('exit'..) code. on('close') should be sufficient. Tested it under some load (30 concurrent connections for 2 minutes) - no errors, 5000+ PDFs rendered. --- lib/pdf.js | 7 +++++-- package.json | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/pdf.js b/lib/pdf.js index 46f2ce4..764bf80 100644 --- a/lib/pdf.js +++ b/lib/pdf.js @@ -123,12 +123,15 @@ PDF.prototype.exec = function PdfExec (callback) { // Ignore if code has a value of 0 since that means PhantomJS has executed and exited successfully. // Also, as per your script and standards, having a code value of 1 means one can always assume that // an error occured. - if (((typeof code !== 'undefined' && code !== null) && code !== 0) || err) { + if (((typeof code !== 'undefined' && code !== null) && code !== 0) || err || (code === 0 && !data)) { var error = null if (err) { // Rudimentary checking if err is an instance of the Error class error = err instanceof Error ? err : new Error(err) + } else if (code === 0 && !data) { + // This is to catch the edge case of having a exit code value of 0 but having no data (exit can be called before io pipes are closed) + error = new Error('html-pdf: Process exited successfully, but no data received') } else { // This is to catch the edge case of having a exit code value of 1 but having no error error = new Error('html-pdf: Unknown Error') @@ -150,7 +153,7 @@ PDF.prototype.exec = function PdfExec (callback) { // An exit event is most likely an error because we didn't get any data at this point child.on('close', respond) - child.on('exit', respond) + // child.on('exit', respond) var config = JSON.stringify({html: this.html, options: this.options}) child.stdin.write(config + '\n', 'utf8') diff --git a/package.json b/package.json index 39e319c..1104756 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "html-pdf", - "version": "3.0.1", + "version": "3.0.2-kv", "description": "HTML to PDF converter that uses phantomjs", "engines": { "node": ">=4.0.0" From d35c703146259a5ab2af97e1d2ece46fddea77d7 Mon Sep 17 00:00:00 2001 From: Michael Salaverry Date: Tue, 8 Feb 2022 19:35:29 +0200 Subject: [PATCH 2/2] test: add test for PR 644 --- lib/pdf.js | 9 ++++++--- test/index.js | 10 ++++++++++ test/phantomMock.js | 3 +++ 3 files changed, 19 insertions(+), 3 deletions(-) create mode 100755 test/phantomMock.js diff --git a/lib/pdf.js b/lib/pdf.js index 46f2ce4..ba07c09 100644 --- a/lib/pdf.js +++ b/lib/pdf.js @@ -123,14 +123,17 @@ PDF.prototype.exec = function PdfExec (callback) { // Ignore if code has a value of 0 since that means PhantomJS has executed and exited successfully. // Also, as per your script and standards, having a code value of 1 means one can always assume that // an error occured. - if (((typeof code !== 'undefined' && code !== null) && code !== 0) || err) { + if (((typeof code !== 'undefined' && code !== null) && code !== 0) || err || (code === 0 && !data)) { var error = null if (err) { // Rudimentary checking if err is an instance of the Error class error = err instanceof Error ? err : new Error(err) + } else if (code === 0 && !data) { + // This is to catch the edge case of having an exit code value of 0 but not having data (exit can be called before io pipes are closed) + error = new Error('html-pdf: Process exited successfully, but no data received') } else { - // This is to catch the edge case of having a exit code value of 1 but having no error + // This is to catch the edge case of having an exit code value of 1 but having no error error = new Error('html-pdf: Unknown Error') } @@ -150,7 +153,7 @@ PDF.prototype.exec = function PdfExec (callback) { // An exit event is most likely an error because we didn't get any data at this point child.on('close', respond) - child.on('exit', respond) + // child.on('exit', respond) var config = JSON.stringify({html: this.html, options: this.options}) child.stdin.write(config + '\n', 'utf8') diff --git a/test/index.js b/test/index.js index bf98a9b..ae67ec1 100644 --- a/test/index.js +++ b/test/index.js @@ -258,3 +258,13 @@ test('allows local file access with localUrlAccess=true', function (t) { t.assert(count === 5, 'Renders a page 5 pages as the content is present') }) }) + +test('phantomjs exit without file generated does not cause crash', function (t) { + t.plan(2) + + pdf.create(`foo`, { phantomPath: './test/phantomMock.js' }) + .toBuffer(function (error, buffer) { + t.true(error instanceof Error) + t.false(buffer) + }) +}) diff --git a/test/phantomMock.js b/test/phantomMock.js new file mode 100755 index 0000000..49d0f0d --- /dev/null +++ b/test/phantomMock.js @@ -0,0 +1,3 @@ +#!/usr/bin/env node + +process.exit(0)