Close file handles/descriptors properly (#367)

lib/index.ts

@@ -62,16 +62,18 @@ function lookup(buffer: Buffer, filepath?: string): ISizeCalculationResult {
  */
 async function asyncFileToBuffer(filepath: string): Promise<Buffer> {
   const handle = await fs.promises.open(filepath, 'r')
-  const { size } = await handle.stat()
-  if (size <= 0) {
+  try {
+    const { size } = await handle.stat()
+    if (size <= 0) {
+      throw new Error('Empty file')
+    }
+    const bufferSize = Math.min(size, MaxBufferSize)
+    const buffer = Buffer.alloc(bufferSize)
+    await handle.read(buffer, 0, bufferSize, 0)
+    return buffer
+  } finally {
     await handle.close()
-    throw new Error('Empty file')
   }
-  const bufferSize = Math.min(size, MaxBufferSize)
-  const buffer = Buffer.alloc(bufferSize)
-  await handle.read(buffer, 0, bufferSize, 0)
-  await handle.close()
-  return buffer
 }
 
 /**
@@ -83,16 +85,18 @@ async function asyncFileToBuffer(filepath: string): Promise<Buffer> {
 function syncFileToBuffer(filepath: string): Buffer {
   // read from the file, synchronously
   const descriptor = fs.openSync(filepath, 'r')
-  const { size } = fs.fstatSync(descriptor)
-  if (size <= 0) {
+  try {
+    const { size } = fs.fstatSync(descriptor)
+    if (size <= 0) {
+      throw new Error('Empty file')
+    }
+    const bufferSize = Math.min(size, MaxBufferSize)
+    const buffer = Buffer.alloc(bufferSize)
+    fs.readSync(descriptor, buffer, 0, bufferSize, 0)
+    return buffer
+  } finally {
     fs.closeSync(descriptor)
-    throw new Error('Empty file')
   }
-  const bufferSize = Math.min(size, MaxBufferSize)
-  const buffer = Buffer.alloc(bufferSize)
-  fs.readSync(descriptor, buffer, 0, bufferSize, 0)
-  fs.closeSync(descriptor)
-  return buffer
 }
 
 // eslint-disable-next-line @typescript-eslint/no-use-before-define

specs/fs-close.spec.ts

@@ -3,24 +3,29 @@ import * as sinon from 'sinon'
 import * as fs from 'fs'
 import { imageSize } from '../lib'
 
-describe('after done reading from files', () => {
-  const readFromClosed = (fd: number) => fs.readSync(fd, Buffer.alloc(1), 0, 1, 0)
+const testBuf = Buffer.alloc(1)
+const readFromClosed = (fd: number) => fs.readSync(fd, testBuf, 0, 1, 0)
 
+describe('after done reading from files', () => {
   describe('should close the file descriptor', () => {
     it('async', done => {
       const spy = sinon.spy(fs.promises, 'open')
-      imageSize('specs/images/valid/jpg/large.jpg', () => {
-        expect(spy.calledOnce).to.be.true
-        const fsPromise = spy.returnValues[0]
-        fsPromise.then((handle) => {
-          expect(() => readFromClosed(handle.fd)).to.throw(Error)
-          spy.restore()
+      imageSize('specs/images/valid/jpg/large.jpg', async err => {
+        try {
+          expect(err).to.be.null
+          expect(spy.calledOnce).to.be.true
+          const fileHandle = await spy.returnValues[0]
+          expect(() => readFromClosed(fileHandle.fd)).to.throw(Error)
+
           done()
-        })
+        } catch (err) {
+          done(err)
+        } finally {
+          spy.restore()
+        }
       })
     })
 
-    // TODO: revisit this spec. how to ensure that we never leave descriptors open
     it('sync', () => {
       const spy = sinon.spy(fs, 'openSync')
       imageSize('specs/images/valid/jpg/large.jpg')
@@ -29,3 +34,45 @@ describe('after done reading from files', () => {
     })
   })
 })
+
+describe('when buffer allocation fails', () => {
+  const sandbox = sinon.createSandbox()
+
+  before(() => {
+    sandbox
+      .stub(Buffer, 'alloc')
+      // Error like the one thrown by Buffer.alloc when there is not enough free memory
+      .throws(new RangeError('Array buffer allocation failed'))
+  })
+
+  after(() => {
+    sandbox.restore()
+  })
+
+  describe('should close the file descriptor', () => {
+    it('async', done => {
+      const spy = sinon.spy(fs.promises, 'open')
+      imageSize('specs/images/valid/jpg/large.jpg', async err => {
+        try {
+          expect(err).to.be.instanceOf(RangeError)
+          expect(spy.calledOnce).to.be.true
+          const fileHandle = await spy.returnValues[0]
+          expect(() => readFromClosed(fileHandle.fd)).to.throw(Error)
+
+          done()
+        } catch (err) {
+          done(err)
+        } finally {
+          spy.restore()
+        }
+      })
+    })
+
+    it('sync', () => {
+      const spy = sinon.spy(fs, 'openSync')
+      expect(() => imageSize('specs/images/valid/jpg/large.jpg')).to.throw(RangeError)
+      expect(() => readFromClosed(spy.returnValues[0])).to.throw(Error, 'bad file descriptor')
+      spy.restore()
+    })
+  })
+})