From 50f967e788f362da57d782829712542c8f15f8c8 Mon Sep 17 00:00:00 2001 From: Francis Gulotta Date: Sat, 9 Nov 2019 21:46:21 -0500 Subject: [PATCH] fix: bindings.close() should cause a canceled read error (#1972) `stream` seems unaffected by this but other future interfaces may not be so forgiving. - removed the unused disconnect handler that we passed to bindings in tests. - moved shouldReject to a test global - unit tested unixRead --- .eslintrc | 3 +- packages/binding-mock/lib/index.js | 10 ++- packages/binding-mock/lib/index.test.js | 12 --- packages/bindings/lib/bindings.test.js | 106 ++++++++---------------- packages/bindings/lib/darwin.js | 2 +- packages/bindings/lib/linux.js | 2 +- packages/bindings/lib/unix-read.js | 28 ++++--- packages/bindings/lib/unix-read.test.js | 104 +++++++++++++++++++++++ packages/stream/lib/index.test.js | 15 +++- test/initializers/assert-reject.js | 9 ++ 10 files changed, 191 insertions(+), 100 deletions(-) create mode 100644 packages/bindings/lib/unix-read.test.js create mode 100644 test/initializers/assert-reject.js diff --git a/.eslintrc b/.eslintrc index 36f4b9b7d..c99be9415 100644 --- a/.eslintrc +++ b/.eslintrc @@ -22,7 +22,8 @@ }, "globals": { "assert": false, - "makeTestFeature": false + "makeTestFeature": false, + "shouldReject": false }, "rules": { "no-extra-semi": "off", diff --git a/packages/binding-mock/lib/index.js b/packages/binding-mock/lib/index.js index 1759e1486..ac6ba25b5 100644 --- a/packages/binding-mock/lib/index.js +++ b/packages/binding-mock/lib/index.js @@ -9,6 +9,12 @@ function resolveNextTick(value) { return new Promise(resolve => process.nextTick(() => resolve(value))) } +const cancelError = message => { + const err = new Error(message) + err.canceled = true + return err +} + /** * Mock bindings for pretend serialport access */ @@ -127,7 +133,7 @@ class MockBinding extends AbstractBinding { delete this.serialNumber this.isOpen = false if (this.pendingRead) { - this.pendingRead(new Error('port is closed')) + this.pendingRead(cancelError('port is closed')) } } @@ -136,7 +142,7 @@ class MockBinding extends AbstractBinding { await super.read(buffer, offset, length) await resolveNextTick() if (!this.isOpen) { - throw new Error('Read canceled') + throw cancelError('Read canceled') } if (this.port.data.length <= 0) { return new Promise((resolve, reject) => { diff --git a/packages/binding-mock/lib/index.test.js b/packages/binding-mock/lib/index.test.js index 2a68aafd8..32e5ae8c3 100644 --- a/packages/binding-mock/lib/index.test.js +++ b/packages/binding-mock/lib/index.test.js @@ -1,17 +1,5 @@ const BindingMock = require('../') -// copypasta from bindings/lib/bindings.test.js -function shouldReject(promise, errType = Error, message = 'Should have rejected') { - return promise.then( - () => { - throw new Error(message) - }, - err => { - assert.instanceOf(err, errType) - } - ) -} - describe('BindingMock', () => { it('constructs', () => { new BindingMock({}) diff --git a/packages/bindings/lib/bindings.test.js b/packages/bindings/lib/bindings.test.js index aefb40bda..5ecebefac 100644 --- a/packages/bindings/lib/bindings.test.js +++ b/packages/bindings/lib/bindings.test.js @@ -35,22 +35,6 @@ const listFields = ['path', 'manufacturer', 'serialNumber', 'pnpId', 'locationId const bindingsToTest = ['mock', platform] -function disconnect(err) { - throw err || new Error('Unknown disconnection') -} - -const shouldReject = (promise, errType = Error, message = 'Should have rejected') => { - return promise.then( - () => { - throw new Error(message) - }, - err => { - assert.instanceOf(err, errType) - return err - } - ) -} - // All bindings are required to work with an "echo" firmware // The echo firmware should respond with this data when it's // ready to echo. This allows for remote device bootup. @@ -108,9 +92,7 @@ function testBinding(bindingName, Binding, testPort) { describe('constructor', () => { it('creates a binding object', () => { - const binding = new Binding({ - disconnect, - }) + const binding = new Binding({}) assert.instanceOf(binding, Binding) }) @@ -133,9 +115,7 @@ function testBinding(bindingName, Binding, testPort) { let binding beforeEach(() => { - binding = new Binding({ - disconnect, - }) + binding = new Binding({}) }) it('is true after open and false after close', () => { @@ -154,9 +134,7 @@ function testBinding(bindingName, Binding, testPort) { describe('#open', () => { let binding beforeEach(() => { - binding = new Binding({ - disconnect, - }) + binding = new Binding({}) }) it('errors when providing a bad port', async () => { @@ -213,7 +191,7 @@ function testBinding(bindingName, Binding, testPort) { describe('optional locking', () => { it('locks the port by default', async () => { - const binding2 = new Binding({ disconnect }) + const binding2 = new Binding({}) await binding.open(testPort, defaultOpenOptions) assert.equal(binding.isOpen, true) await shouldReject(binding2.open(testPort, defaultOpenOptions)) @@ -223,7 +201,7 @@ function testBinding(bindingName, Binding, testPort) { testFeature('open.unlock', 'can unlock the port', async () => { const noLock = { ...defaultOpenOptions, lock: false } - const binding2 = new Binding({ disconnect }) + const binding2 = new Binding({}) await binding.open(testPort, noLock) assert.equal(binding.isOpen, true) @@ -239,7 +217,7 @@ function testBinding(bindingName, Binding, testPort) { describe('#close', () => { let binding beforeEach(() => { - binding = new Binding({ disconnect }) + binding = new Binding({}) }) it('errors when already closed', async () => { @@ -261,14 +239,12 @@ function testBinding(bindingName, Binding, testPort) { describe('#update', () => { it('throws when not given an object', async () => { - const binding = new Binding({ disconnect }) + const binding = new Binding({}) await shouldReject(binding.update(), TypeError) }) it('errors asynchronously when not open', done => { - const binding = new Binding({ - disconnect, - }) + const binding = new Binding({}) let noZalgo = false binding.update({ baudRate: 9600 }).catch(err => { assert.instanceOf(err, Error) @@ -285,7 +261,7 @@ function testBinding(bindingName, Binding, testPort) { let binding beforeEach(() => { - binding = new Binding({ disconnect }) + binding = new Binding({}) return binding.open(testPort, defaultOpenOptions) }) @@ -306,9 +282,7 @@ function testBinding(bindingName, Binding, testPort) { describe('#write', () => { it('errors asynchronously when not open', done => { - const binding = new Binding({ - disconnect, - }) + const binding = new Binding({}) let noZalgo = false binding .write(Buffer.from([])) @@ -328,9 +302,7 @@ function testBinding(bindingName, Binding, testPort) { }) it('rejects when not given a buffer', async () => { - const binding = new Binding({ - disconnect, - }) + const binding = new Binding({}) await shouldReject(binding.write(null), TypeError) }) @@ -341,9 +313,7 @@ function testBinding(bindingName, Binding, testPort) { let binding beforeEach(() => { - binding = new Binding({ - disconnect, - }) + binding = new Binding({}) return binding.open(testPort, defaultOpenOptions) }) @@ -368,9 +338,7 @@ function testBinding(bindingName, Binding, testPort) { describe('#drain', () => { it('errors asynchronously when not open', done => { - const binding = new Binding({ - disconnect, - }) + const binding = new Binding({}) let noZalgo = false binding.drain().catch(err => { assert.instanceOf(err, Error) @@ -387,9 +355,7 @@ function testBinding(bindingName, Binding, testPort) { let binding beforeEach(() => { - binding = new Binding({ - disconnect, - }) + binding = new Binding({}) return binding.open(testPort, defaultOpenOptions) }) @@ -418,9 +384,7 @@ function testBinding(bindingName, Binding, testPort) { describe('#flush', () => { it('errors asynchronously when not open', done => { - const binding = new Binding({ - disconnect, - }) + const binding = new Binding({}) let noZalgo = false binding.flush().catch(err => { assert.instanceOf(err, Error) @@ -437,9 +401,7 @@ function testBinding(bindingName, Binding, testPort) { let binding beforeEach(() => { - binding = new Binding({ - disconnect, - }) + binding = new Binding({}) return binding.open(testPort, defaultOpenOptions) }) @@ -452,9 +414,7 @@ function testBinding(bindingName, Binding, testPort) { describe('#set', () => { it('errors asynchronously when not open', done => { - const binding = new Binding({ - disconnect, - }) + const binding = new Binding({}) let noZalgo = false binding.set(defaultSetFlags).catch(err => { assert.instanceOf(err, Error) @@ -465,9 +425,7 @@ function testBinding(bindingName, Binding, testPort) { }) it('throws when not called with options', async () => { - const binding = new Binding({ - disconnect, - }) + const binding = new Binding({}) await shouldReject(binding.set(() => {}), TypeError) }) @@ -478,9 +436,7 @@ function testBinding(bindingName, Binding, testPort) { let binding beforeEach(() => { - binding = new Binding({ - disconnect, - }) + binding = new Binding({}) return binding.open(testPort, defaultOpenOptions) }) @@ -495,7 +451,7 @@ function testBinding(bindingName, Binding, testPort) { // is left over on the pipe and isn't cleared when flushed on unix describe('#read', () => { it('errors asynchronously when not open', done => { - const binding = new Binding({ disconnect }) + const binding = new Binding({}) const buffer = Buffer.alloc(5) let noZalgo = false binding.read(buffer, 0, buffer.length).catch(err => { @@ -514,11 +470,11 @@ function testBinding(bindingName, Binding, testPort) { let binding, buffer beforeEach(async () => { buffer = Buffer.alloc(readyData.length) - binding = new Binding({ disconnect }) + binding = new Binding({}) await binding.open(testPort, defaultOpenOptions) }) - afterEach(() => binding.close()) + afterEach(() => binding.isOpen && binding.close()) it("doesn't throw if the port is open", async () => { await binding.read(buffer, 0, buffer.length) @@ -529,13 +485,23 @@ function testBinding(bindingName, Binding, testPort) { assert.equal(bytesRead, 1) assert.equal(buffer, returnedBuffer) }) + + it('cancels the read when the port is closed', async () => { + let bytesToRead = 0 + while (bytesToRead < readyData.length) { + const { bytesRead } = await binding.read(buffer, 0, readyData.length) + bytesToRead += bytesRead + } + const readError = shouldReject(binding.read(Buffer.allocUnsafe(100), 0, 100)) + await binding.close() + const err = await readError + assert.isTrue(err.canceled) + }) }) describe('#get', () => { it('errors asynchronously when not open', done => { - const binding = new Binding({ - disconnect, - }) + const binding = new Binding({}) let noZalgo = false binding.get().catch(err => { assert.instanceOf(err, Error) @@ -552,7 +518,7 @@ function testBinding(bindingName, Binding, testPort) { let binding beforeEach(() => { - binding = new Binding({ disconnect }) + binding = new Binding({}) return binding.open(testPort, defaultOpenOptions) }) diff --git a/packages/bindings/lib/darwin.js b/packages/bindings/lib/darwin.js index c8d74c6ab..0bce5f044 100644 --- a/packages/bindings/lib/darwin.js +++ b/packages/bindings/lib/darwin.js @@ -61,7 +61,7 @@ class DarwinBinding extends AbstractBinding { async read(buffer, offset, length) { await super.read(buffer, offset, length) - return unixRead.call(this, buffer, offset, length) + return unixRead({ binding: this, buffer, offset, length }) } async write(buffer) { diff --git a/packages/bindings/lib/linux.js b/packages/bindings/lib/linux.js index c56889007..e089e819c 100644 --- a/packages/bindings/lib/linux.js +++ b/packages/bindings/lib/linux.js @@ -61,7 +61,7 @@ class LinuxBinding extends AbstractBinding { async read(buffer, offset, length) { await super.read(buffer, offset, length) - return unixRead.call(this, buffer, offset, length) + return unixRead({ binding: this, buffer, offset, length }) } async write(buffer) { diff --git a/packages/bindings/lib/unix-read.js b/packages/bindings/lib/unix-read.js index dbf443451..fe6bda557 100644 --- a/packages/bindings/lib/unix-read.js +++ b/packages/bindings/lib/unix-read.js @@ -11,28 +11,32 @@ const readable = binding => { }) } -module.exports = async function unixRead(buffer, offset, length) { +const unixRead = async ({ binding, buffer, offset, length, fsReadAsync = readAsync }) => { logger('Starting read') - if (!this.isOpen) { - throw new Error('Port is not open') + if (!binding.isOpen) { + const err = new Error('Port is not open') + err.canceled = true + throw err } try { - const { bytesRead } = await readAsync(this.fd, buffer, offset, length, null) + const { bytesRead } = await fsReadAsync(binding.fd, buffer, offset, length, null) if (bytesRead === 0) { - return this.read(buffer, offset, length) + return unixRead({ binding, buffer, offset, length, fsReadAsync }) } - logger('Finished read', bytesRead, 'bytes') return { bytesRead, buffer } } catch (err) { + logger('read error', err) if (err.code === 'EAGAIN' || err.code === 'EWOULDBLOCK' || err.code === 'EINTR') { - if (!this.isOpen) { - throw new Error('Port is not open') + if (!binding.isOpen) { + const err = new Error('Port is not open') + err.canceled = true + throw err } logger('waiting for readable because of code:', err.code) - await readable(this) - return this.read(buffer, offset, length) + await readable(binding) + return unixRead({ binding, buffer, offset, length, fsReadAsync }) } const disconnectError = @@ -42,10 +46,12 @@ module.exports = async function unixRead(buffer, offset, length) { err.errno === -1 // generic error if (disconnectError) { - err.disconnect = true + err.canceled = true logger('disconnecting', err) } throw err } } + +module.exports = unixRead diff --git a/packages/bindings/lib/unix-read.test.js b/packages/bindings/lib/unix-read.test.js new file mode 100644 index 000000000..f5da9a5b9 --- /dev/null +++ b/packages/bindings/lib/unix-read.test.js @@ -0,0 +1,104 @@ +const unixRead = require('./unix-read') + +const makeFsRead = (bytesRead, fill) => (fd, buffer, offset, length) => { + buffer.fill(fill, offset, Math.min(length, bytesRead)) + return { + buffer, + bytesRead, + } +} + +const makeFsReadError = code => { + const err = new Error(`Error: ${code}`) + err.code = code + return () => { + throw err + } +} + +const sequenceCalls = (...functions) => { + const funcs = [...functions] + return (...args) => { + const func = funcs.shift() + if (func) { + return func(...args) + } else { + throw new Error('"delegateCalls" has no more functions') + } + } +} + +const makeMockBinding = () => { + return { + isOpen: true, + fd: 1, + poller: { + once(event, func) { + setImmediate(func) + }, + }, + } +} + +describe('unix-read', () => { + let mock + beforeEach(() => { + mock = makeMockBinding() + }) + it('rejects when not open', async () => { + mock.isOpen = false + const readBuffer = Buffer.alloc(8, 0) + await shouldReject(unixRead({ binding: mock, buffer: readBuffer, offset: 0, length: 8, fsReadAsync: makeFsRead(8, 255) })) + }) + it('handles reading the requested number of bytes', async () => { + const readBuffer = Buffer.alloc(8, 0) + const { bytesRead, buffer } = await unixRead({ binding: mock, buffer: readBuffer, offset: 0, length: 8, fsReadAsync: makeFsRead(8, 255) }) + assert.strictEqual(bytesRead, 8) + assert.strictEqual(buffer, readBuffer) + assert.deepStrictEqual(buffer, Buffer.alloc(8, 255)) + }) + it('handles reading less than requested number of bytes', async () => { + const readBuffer = Buffer.alloc(8, 0) + const { bytesRead, buffer } = await unixRead({ binding: mock, buffer: readBuffer, offset: 0, length: 8, fsReadAsync: makeFsRead(4, 255) }) + assert.strictEqual(bytesRead, 4) + assert.strictEqual(buffer, readBuffer) + assert.deepStrictEqual(buffer, Buffer.from([255, 255, 255, 255, 0, 0, 0, 0])) + }) + it('handles reading 0 bytes then requested number of bytes', async () => { + const readBuffer = Buffer.alloc(8, 0) + + const fsReadAsync = sequenceCalls(makeFsRead(0, 0), makeFsRead(8, 255)) + const { bytesRead, buffer } = await unixRead({ binding: mock, buffer: readBuffer, offset: 0, length: 8, fsReadAsync }) + assert.strictEqual(bytesRead, 8) + assert.strictEqual(buffer, readBuffer) + assert.deepStrictEqual(buffer, Buffer.alloc(8, 255)) + }) + it('handles retryable errors', async () => { + const readBuffer = Buffer.alloc(8, 0) + + const fsReadAsync = sequenceCalls(makeFsReadError('EAGAIN'), makeFsReadError('EWOULDBLOCK'), makeFsReadError('EINTR'), makeFsRead(8, 255)) + const { bytesRead, buffer } = await unixRead({ binding: mock, buffer: readBuffer, offset: 0, length: 8, fsReadAsync }) + assert.strictEqual(bytesRead, 8) + assert.strictEqual(buffer, readBuffer) + assert.deepStrictEqual(buffer, Buffer.alloc(8, 255)) + }) + it('rejects read errors', async () => { + const readBuffer = Buffer.alloc(8, 0) + await shouldReject(unixRead({ binding: mock, buffer: readBuffer, offset: 0, length: 8, fsReadAsync: makeFsReadError('Error') })) + }) + it('rejects a canceled error if port closes after read a retryable error', async () => { + const readBuffer = Buffer.alloc(8, 0) + const fsReadAsync = () => { + mock.isOpen = false + makeFsReadError('EAGAIN')() + } + const err = await shouldReject(unixRead({ binding: mock, buffer: readBuffer, offset: 0, length: 8, fsReadAsync })) + assert.isTrue(err.canceled) + }) + it('rejects a canceled error read errors a disconnect error', async () => { + const readBuffer = Buffer.alloc(8, 0) + const fsReadAsync = makeFsReadError('EBADF') + const err = await shouldReject(unixRead({ binding: mock, buffer: readBuffer, offset: 0, length: 8, fsReadAsync })) + assert.isTrue(err.canceled) + }) +}) diff --git a/packages/stream/lib/index.test.js b/packages/stream/lib/index.test.js index 120d8c2aa..717e4e056 100644 --- a/packages/stream/lib/index.test.js +++ b/packages/stream/lib/index.test.js @@ -974,13 +974,24 @@ describe('SerialPort', () => { const data2 = port.read() assert.deepEqual(Buffer.concat([data1, data2]), testData) }) + + it("doesn't error if the port is closed when reading", async () => { + const port = new SerialPort('/dev/exists') + await new Promise(resolve => port.on('open', resolve)) + port.read() + port.read() + let err = null + port.on('error', error => (err = error)) + await new Promise((resolve, reject) => port.close(err => (err ? reject(err) : resolve()))) + assert.isNull(err) + }) }) describe('disconnect close errors', () => { it('emits as a disconnected close event on a bad read', done => { const port = new SerialPort('/dev/exists') - sinon.stub(port.binding, 'read').callsFake(() => { - return Promise.reject(new Error('EBAD_ERR')) + sinon.stub(port.binding, 'read').callsFake(async () => { + throw new Error('EBAD_ERR') }) port.on('close', err => { assert.instanceOf(err, Error) diff --git a/test/initializers/assert-reject.js b/test/initializers/assert-reject.js new file mode 100644 index 000000000..ceea60c74 --- /dev/null +++ b/test/initializers/assert-reject.js @@ -0,0 +1,9 @@ +global.shouldReject = async (promise, errType = Error, message = 'Should have rejected') => { + try { + await promise + } catch (err) { + assert.instanceOf(err, errType) + return err + } + throw new Error(message) +}