Skip to content

Commit

Permalink
fix: bindings.close() should cause a canceled read error (#1972)
Browse files Browse the repository at this point in the history 
`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
  • Loading branch information
@reconbot
reconbot committed Nov 10, 2019
1 parent e978b7e commit 50f967e
Show file tree
Hide file tree
Showing 10 changed files with 191 additions and 100 deletions.
3 changes: 2 additions & 1 deletion .eslintrc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
},
"globals": {
"assert": false,
"makeTestFeature": false
"makeTestFeature": false,
"shouldReject": false
},
"rules": {
"no-extra-semi": "off",
Expand Down
10 changes: 8 additions & 2 deletions packages/binding-mock/lib/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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'))
}
}

Expand All @@ -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) => {
Expand Down
12 changes: 0 additions & 12 deletions packages/binding-mock/lib/index.test.js
View file
Original file line number Diff line number Diff line change
@@ -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({})
Expand Down
106 changes: 36 additions & 70 deletions packages/bindings/lib/bindings.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
})

Expand All @@ -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', () => {
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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))
Expand All @@ -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)
Expand All @@ -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 () => {
Expand All @@ -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)
Expand All @@ -285,7 +261,7 @@ function testBinding(bindingName, Binding, testPort) {

let binding
beforeEach(() => {
binding = new Binding({ disconnect })
binding = new Binding({})
return binding.open(testPort, defaultOpenOptions)
})

Expand All @@ -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([]))
Expand All @@ -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)
})

Expand All @@ -341,9 +313,7 @@ function testBinding(bindingName, Binding, testPort) {

let binding
beforeEach(() => {
binding = new Binding({
disconnect,
})
binding = new Binding({})
return binding.open(testPort, defaultOpenOptions)
})

Expand All @@ -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)
Expand All @@ -387,9 +355,7 @@ function testBinding(bindingName, Binding, testPort) {

let binding
beforeEach(() => {
binding = new Binding({
disconnect,
})
binding = new Binding({})
return binding.open(testPort, defaultOpenOptions)
})

Expand Down Expand Up @@ -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)
Expand All @@ -437,9 +401,7 @@ function testBinding(bindingName, Binding, testPort) {

let binding
beforeEach(() => {
binding = new Binding({
disconnect,
})
binding = new Binding({})
return binding.open(testPort, defaultOpenOptions)
})

Expand All @@ -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)
Expand All @@ -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)
})

Expand All @@ -478,9 +436,7 @@ function testBinding(bindingName, Binding, testPort) {

let binding
beforeEach(() => {
binding = new Binding({
disconnect,
})
binding = new Binding({})
return binding.open(testPort, defaultOpenOptions)
})

Expand All @@ -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 => {
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -552,7 +518,7 @@ function testBinding(bindingName, Binding, testPort) {

let binding
beforeEach(() => {
binding = new Binding({ disconnect })
binding = new Binding({})
return binding.open(testPort, defaultOpenOptions)
})

Expand Down
2 changes: 1 addition & 1 deletion packages/bindings/lib/darwin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion packages/bindings/lib/linux.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit 50f967e

Please sign in to comment.