Clean up image API code paths (#49)

* Document image.js a bit

* Close image.js sockets in all code paths

I'm not sure whether sockets get GC'd when the function returns

* Remove getFormat

It was only called from one place, and the object property names were
quite confusing

* Clean up image.js conditional a bit

I had to write out an entire truth table for this and work it all out
Thinking hard

* Move actual ImageMagick calling into separate file

This gets rid of the weird, brain-melting ouroboros of code that
recurses across threads and processes.

* Reduce amount of getType wrangling

This amounted to an awful lot of dead conditionals after the image
commands were all modified to pass in image types anyway. This has also
led to two different implementations diverging, which causes bugs like
GIF commands applied to non-GIFs erroring instead of providing a
user-friendly message.

* Unify image-runner return type, clarify image type

This allows us to remove the fromAPI parameter from image-runner, and
helps greatly clarify the behavior around image types.

* Deduplicate GIF code, fix "not a GIF" handling

The special "nogif" value is now stored as the image type instead of its
value, as the value must always be a Buffer now--no loosely-typed
shenanigans.

api/index.js

@@ -2,8 +2,7 @@
 
 require("dotenv").config();
 const os = require("os");
-const magick = require("../utils/image.js");
+const { run } = require("../utils/image-runner.js");
-const execPromise = require("util").promisify(require("child_process").exec);
 const net = require("net");
 const dgram = require("dgram"); // for UDP servers
 const socket = dgram.createSocket("udp4"); // Our universal UDP socket, this might cause issues and we may have to use a seperate socket for each connection
@@ -95,31 +94,17 @@ const runJob = (job) => {
     log(`Job ${job.uuid} starting...`, job.num);
 
     const object = JSON.parse(job.msg);
-    let type;
+    // If the image has a path, it must also have a type
-    if (object.path) {
+    if (object.path && !object.type) {
-      type = object.type;
-      if (!object.type) {
-        type = await magick.getType(object.path);
-      }
-      if (!type) {
       reject(new TypeError("Unknown image type"));
     }
-      object.type = type.split("/")[1];
-      if (object.type !== "gif" && object.onlyGIF) reject(new TypeError(`Expected a GIF, got ${object.type}`));
-      object.delay = object.delay ? object.delay : 0;
-    }
-      
-    if (object.type === "gif" && !object.delay) {
-      const delay = (await execPromise(`ffprobe -v 0 -of csv=p=0 -select_streams v:0 -show_entries stream=r_frame_rate ${object.path}`)).stdout.replace("\n", "");
-      object.delay = (100 / delay.split("/")[0]) * delay.split("/")[1];
-    }
 
     log(`Job ${job.uuid} started`, job.num);
-    const data = await magick.run(object, true);
+    const {buffer, fileExtension} = await run(object);
 
     log(`Sending result of job ${job.uuid} back to the bot`, job.num);
     const server = net.createServer(function(tcpSocket) {
-      tcpSocket.write(Buffer.concat([Buffer.from(type ? type : "image/png"), Buffer.from("\n"), data]), (err) => {
+      tcpSocket.write(Buffer.concat([Buffer.from(fileExtension), Buffer.from("\n"), buffer]), (err) => {
         if (err) console.error(err);
         tcpSocket.end(() => {
           server.close();

commands/freeze.js

@@ -11,7 +11,7 @@ exports.run = async (message) => {
     onlyGIF: true,
     type: image.type
   });
-  if (buffer === "nogif") return `${message.author.mention}, that isn't a GIF!`;
+  if (type === "nogif") return `${message.author.mention}, that isn't a GIF!`;
   return {
     file: buffer,
     name: `freeze.${type}`

commands/reverse.js

@@ -11,7 +11,7 @@ exports.run = async (message) => {
     onlyGIF: true,
     type: image.type
   });
-  if (buffer === "nogif") return `${message.author.mention}, that isn't a GIF!`;
+  if (type === "nogif") return `${message.author.mention}, that isn't a GIF!`;
   return {
     file: buffer,
     name: `reverse.${type}`

commands/slow.js

@@ -11,7 +11,7 @@ exports.run = async (message) => {
     onlyGIF: true,
     type: image.type
   });
-  if (buffer === "nogif") return `${message.author.mention}, that isn't a GIF!`;
+  if (type === "nogif") return `${message.author.mention}, that isn't a GIF!`;
   return {
     file: buffer,
     name: `slow.${type}`

commands/soos.js

@@ -12,7 +12,7 @@ exports.run = async (message) => {
     onlyGIF: true,
     type: image.type
   });
-  if (buffer === "nogif") return `${message.author.mention}, that isn't a GIF!`;
+  if (type === "nogif") return `${message.author.mention}, that isn't a GIF!`;
   return {
     file: buffer,
     name: `soos.${type}`

commands/speed.js

@@ -10,7 +10,7 @@ exports.run = async (message) => {
     onlyGIF: true,
     type: image.type
   });
-  if (buffer === "nogif") return `${message.author.mention}, that isn't a GIF!`;
+  if (type === "nogif") return `${message.author.mention}, that isn't a GIF!`;
   return {
     file: buffer,
     name: `speed.${type}`

commands/unfreeze.js

@@ -11,7 +11,7 @@ exports.run = async (message) => {
     onlyGIF: true,
     type: image.type
   });
-  if (buffer === "nogif") return `${message.author.mention}, that isn't a GIF!`;
+  if (type === "nogif") return `${message.author.mention}, that isn't a GIF!`;
   return {
     file: buffer,
     name: `unfreeze.${type}`

utils/image-runner.js

@@ -0,0 +1,42 @@
+const magick = require("../build/Release/image.node");
+const { promisify } = require("util");
+const execPromise = promisify(require("child_process").exec);
+const { isMainThread, parentPort, workerData } = require("worker_threads");
+
+exports.run = async object => {
+  return new Promise(async resolve => {
+    // If the image has a path, it must also have a type
+    if (object.path) {
+      if (object.type !== "image/gif" && object.onlyGIF) resolve({
+        buffer: Buffer.alloc(0),
+        fileExtension: "nogif"
+      });
+      const delay = (await execPromise(`ffprobe -v 0 -of csv=p=0 -select_streams v:0 -show_entries stream=r_frame_rate ${object.path}`)).stdout.replace("\n", "");
+      object.delay = (100 / delay.split("/")[0]) * delay.split("/")[1];
+    }
+    // Convert from a MIME type (e.g. "image/png") to something ImageMagick understands (e.g. "png").
+    // Don't set `type` directly on the object we are passed as it will be read afterwards.
+    // If no image type is given (say, the command generates its own image), make it a PNG.
+    const fileExtension = object.type ? object.type.split("/")[1] : "png";
+    const objectWithFixedType = Object.assign({}, object, {type: fileExtension});
+    const data = await promisify(magick[object.cmd])(objectWithFixedType);
+    const returnObject = {
+      buffer: data,
+      fileExtension
+    };
+    resolve(returnObject);
+  });
+};
+
+if (!isMainThread) {
+  this.run(workerData)
+    // eslint-disable-next-line promise/always-return
+    .then(returnObject => {
+      parentPort.postMessage(returnObject);
+      process.exit();
+    })
+    .catch(err => {
+      // turn promise rejection into normal error
+      throw err;
+    });
+}

utils/image.js

@@ -1,13 +1,12 @@
 const magick = require("../build/Release/image.node");
-const { Worker, isMainThread, parentPort, workerData } = require("worker_threads");
+const { Worker } = require("worker_threads");
 const fetch = require("node-fetch");
-const { promisify } = require("util");
 const AbortController = require("abort-controller");
 const net = require("net");
 const dgram = require("dgram");
 const fileType = require("file-type");
-const execPromise = promisify(require("child_process").exec);
 const servers = require("../servers.json").image;
+const path = require("path");
 
 const formats = ["image/jpeg", "image/png", "image/webp", "image/gif"];
 
@@ -64,17 +63,6 @@ const getIdeal = () => {
   });
 };
 
-const getFormat = (buffer, delimiter) => {
-  for (var i = 0; i < buffer.length ; i++) {
-    if (String.fromCharCode(buffer[i]) === delimiter) {
-      return {
-        buffer: buffer.slice(0, i),
-        dataStart: i
-      };
-    }
-  }
-};
-
 exports.check = (cmd) => {
   return magick[cmd] ? true : false;
 };
@@ -114,14 +102,16 @@ exports.getType = async (image) => {
   return type;
 };
 
-exports.run = (object, fromAPI = false) => {
+exports.run = object => {
   return new Promise(async (resolve, reject) => {
-    if (process.env.API === "true" && !fromAPI) {
+    if (process.env.API === "true") {
+      // Connect to best image server
       const currentServer = await getIdeal();
       const socket = dgram.createSocket("udp4");
-      const data = Buffer.concat([Buffer.from([0x1]), Buffer.from(JSON.stringify(object))]);
+      const data = Buffer.concat([Buffer.from([0x1 /* queue job */]), Buffer.from(JSON.stringify(object))]);
 
       const timeout = setTimeout(() => {
+        socket.close();
         reject("UDP timed out");
       }, 25000);
 
@@ -130,10 +120,11 @@ exports.run = (object, fromAPI = false) => {
         const opcode = msg.readUint8(0);
         const req = msg.slice(37, msg.length);
         const uuid = msg.slice(1, 36).toString();
-        if (opcode === 0x0) {
+        if (opcode === 0x0) { // Job queued
           clearTimeout(timeout);
           jobID = uuid;
-        } else if (opcode === 0x1) {
+        } else if (opcode === 0x1) { // Job completed successfully
+          // the image API sends all job responses over the same socket; make sure this is ours
           if (jobID === uuid) {
             const client = net.createConnection(req.toString(), currentServer.addr);
             const array = [];
@@ -142,63 +133,49 @@ exports.run = (object, fromAPI = false) => {
             });
             client.once("end", () => {
               const data = Buffer.concat(array);
-              const format = getFormat(data, "\n");
+              // The response data is given as the file extension/ImageMagick type of the image (e.g. "png"), followed
-              const payload = {
+              // by a newline, followed by the image data.
-                buffer: data.slice(format.dataStart + 1),
+              const delimIndex = data.indexOf("\n");
-                type: format.buffer.toString().split("/")[1]
-              };
               socket.close();
+              if (delimIndex === -1) reject("Could not parse response");
+              const payload = {
+                // Take just the image data
+                buffer: data.slice(delimIndex + 1),
+                type: data.slice(0, delimIndex).toString()
+              };
               resolve(payload);
             });
             client.on("error", (err) => {
+              socket.close();
               reject(err);
             });
           }
-        } else if (opcode === 0x2) {
+        } else if (opcode === 0x2) { // Job errored
-          if (jobID === uuid) reject(req);
+          if (jobID === uuid) {
+            socket.close();
+            reject(req);
+          }
         }
       });
 
       socket.send(data, 8080, currentServer.addr, (err) => {
-        if (err) reject(err);
+        if (err) {
+          socket.close();
+          reject(err);
+        }
       });
-    } else if (isMainThread && !fromAPI) {
+    } else {
-      const worker = new Worker(__filename, {
+      // Called from command (not using image API)
+      const worker = new Worker(path.join(__dirname, "image-runner.js"), {
         workerData: object
       });
       worker.on("message", (data) => {
         resolve({
           buffer: Buffer.from([...data.buffer]),
-          type: data.type
+          type: data.fileExtension
         });
       });
       worker.on("error", reject);
-    } else {
-      let type;
-      if (!fromAPI && object.path) {
-        const newType = (object.type ? object.type : await this.getType(object.path));
-        type = newType ? newType.split("/")[1] : "png";
-        if (type !== "gif" && object.onlyGIF) resolve({
-          buffer: "nogif",
-          type: null
-        });
-        object.type = type;
-        const delay = (await execPromise(`ffprobe -v 0 -of csv=p=0 -select_streams v:0 -show_entries stream=r_frame_rate ${object.path}`)).stdout.replace("\n", "");
-        object.delay = (100 / delay.split("/")[0]) * delay.split("/")[1];
-      }
-      const data = await promisify(magick[object.cmd])(object);
-      const returnObject = fromAPI ? data : {
-        buffer: data,
-        type: type
-      };
-      if (!isMainThread && !fromAPI) {
-        parentPort.postMessage(returnObject);
-        process.exit();
-      } else {
-        resolve(returnObject);
-      }
     }
   });
 };
-
-if (!isMainThread && process.env.API !== "true") this.run(workerData);