From 64065141b6e4cd2007b035384d24b5a016f08e62 Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Fri, 5 Apr 2024 12:35:47 +0200 Subject: [PATCH 1/2] Remove the timeout logic from the TTX driver code The original `test.py` code, see https://github.com/mozilla/pdf.js/blob/c2376e5cea1b3cee89d77e6a4e8b0d9b5b8591be/test/test.py, did not have any timeout logic for TTX, but it got introduced when `test.py` was ported from Python to JavaScript as `test.js` in https://github.com/mozilla/pdf.js/commit/c2376e5cea1b3cee89d77e6a4e8b0d9b5b8591be#diff-a561630bb56b82342bc66697aee2ad96efddcbc9d150665abd6fb7ecb7c0ab2f. However, I don't think we've ever actually seen TTX timing out. Moreover, back then we used a very old version of TTX and ran the font tests on the bots (where a hanging process would block other jobs and would require a manual action to fix), so this code was most likely only included defensively. Fortunately, nowadays it should not be necessary anymore because we use the most recent version of TTX (which either returns the result or errors out, but isn't known to hang on inputs) and we run the font tests on GitHub Actions which doesn't block other jobs anymore and also automatically times the job out for us in the unlikely event that a hang would ever occur. In short, we can safely remove this logic to simplify the code and to get rid of a callback. --- test/font/ttxdriver.mjs | 11 +++-------- test/test.mjs | 20 ++++---------------- 2 files changed, 7 insertions(+), 24 deletions(-) diff --git a/test/font/ttxdriver.mjs b/test/font/ttxdriver.mjs index c820d8ccf..3e51caa4b 100644 --- a/test/font/ttxdriver.mjs +++ b/test/font/ttxdriver.mjs @@ -21,14 +21,9 @@ import { spawn } from "child_process"; let ttxTaskId = Date.now(); -function runTtx(fontPath, registerOnCancel, callback) { +function runTtx(fontPath, callback) { const ttx = spawn("ttx", [fontPath], { stdio: "ignore" }); let ttxRunError; - registerOnCancel(function (reason) { - ttxRunError = reason; - callback(reason); - ttx.kill(); - }); ttx.on("error", function (errorTtx) { ttxRunError = errorTtx; callback( @@ -43,14 +38,14 @@ function runTtx(fontPath, registerOnCancel, callback) { }); } -function translateFont(content, registerOnCancel, callback) { +function translateFont(content, callback) { const buffer = Buffer.from(content, "base64"); const taskId = (ttxTaskId++).toString(); const fontPath = path.join(os.tmpdir(), `pdfjs-font-test-${taskId}.otf`); const resultPath = path.join(os.tmpdir(), `pdfjs-font-test-${taskId}.ttx`); fs.writeFileSync(fontPath, buffer); - runTtx(fontPath, registerOnCancel, function (err) { + runTtx(fontPath, function (err) { fs.unlinkSync(fontPath); if (err) { console.error(err); diff --git a/test/test.mjs b/test/test.mjs index 1080be89d..a6a1e76b4 100644 --- a/test/test.mjs +++ b/test/test.mjs @@ -842,22 +842,10 @@ function unitTestPostHandler(req, res) { }); req.on("end", function () { if (pathname === "/ttx") { - var onCancel = null, - ttxTimeout = 10000; - var timeoutId = setTimeout(function () { - onCancel?.("TTX timeout"); - }, ttxTimeout); - translateFont( - body, - function (fn) { - onCancel = fn; - }, - function (err, xml) { - clearTimeout(timeoutId); - res.writeHead(200, { "Content-Type": "text/xml" }); - res.end(err ? "" + err + "" : xml); - } - ); + translateFont(body, function (err, xml) { + res.writeHead(200, { "Content-Type": "text/xml" }); + res.end(err ? "" + err + "" : xml); + }); return; } From ac03d7054d0858b8ca259f21a019a59581aec27d Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Fri, 5 Apr 2024 13:03:22 +0200 Subject: [PATCH 2/2] Convert the TTX driver code to promises This commit removes the final callbacks in this code by switching to a promises-based interface, overall simplifying the code. Moreover, we document why we write to files on disk and modernize the code using e.g. template strings. --- test/font/ttxdriver.mjs | 63 +++++++++++++++++++++++------------------ test/test.mjs | 12 ++++---- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/test/font/ttxdriver.mjs b/test/font/ttxdriver.mjs index 3e51caa4b..ba24f20ad 100644 --- a/test/font/ttxdriver.mjs +++ b/test/font/ttxdriver.mjs @@ -21,42 +21,51 @@ import { spawn } from "child_process"; let ttxTaskId = Date.now(); -function runTtx(fontPath, callback) { - const ttx = spawn("ttx", [fontPath], { stdio: "ignore" }); - let ttxRunError; - ttx.on("error", function (errorTtx) { - ttxRunError = errorTtx; - callback( - "Unable to execute `ttx`; make sure the `fonttools` dependency is installed" - ); - }); - ttx.on("close", function (code) { - if (ttxRunError) { - return; - } - callback(); +function runTtx(fontPath) { + return new Promise((resolve, reject) => { + const ttx = spawn("ttx", [fontPath], { stdio: "ignore" }); + ttx.on("error", () => { + reject( + new Error( + "Unable to execute `ttx`; make sure the `fonttools` dependency is installed" + ) + ); + }); + ttx.on("close", () => { + resolve(); + }); }); } -function translateFont(content, callback) { +async function translateFont(content) { const buffer = Buffer.from(content, "base64"); const taskId = (ttxTaskId++).toString(); const fontPath = path.join(os.tmpdir(), `pdfjs-font-test-${taskId}.otf`); const resultPath = path.join(os.tmpdir(), `pdfjs-font-test-${taskId}.ttx`); + // Write the font data to a temporary file on disk (because TTX only accepts + // files as input). fs.writeFileSync(fontPath, buffer); - runTtx(fontPath, function (err) { - fs.unlinkSync(fontPath); - if (err) { - console.error(err); - callback(err); - } else if (!fs.existsSync(resultPath)) { - callback("Output was not generated"); - } else { - callback(null, fs.readFileSync(resultPath)); - fs.unlinkSync(resultPath); - } - }); + + // Run TTX on the temporary font file. + let ttxError; + try { + await runTtx(fontPath); + } catch (error) { + ttxError = error; + } + + // Remove the temporary font/result files and report on the outcome. + fs.unlinkSync(fontPath); + if (ttxError) { + throw ttxError; + } + if (!fs.existsSync(resultPath)) { + throw new Error("TTX did not generate output"); + } + const xml = fs.readFileSync(resultPath); + fs.unlinkSync(resultPath); + return xml; } export { translateFont }; diff --git a/test/test.mjs b/test/test.mjs index a6a1e76b4..9dc0df70f 100644 --- a/test/test.mjs +++ b/test/test.mjs @@ -840,12 +840,14 @@ function unitTestPostHandler(req, res) { req.on("data", function (data) { body += data; }); - req.on("end", function () { + req.on("end", async function () { if (pathname === "/ttx") { - translateFont(body, function (err, xml) { - res.writeHead(200, { "Content-Type": "text/xml" }); - res.end(err ? "" + err + "" : xml); - }); + res.writeHead(200, { "Content-Type": "text/xml" }); + try { + res.end(await translateFont(body)); + } catch (error) { + res.end(`${error}`); + } return; }