Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: bindings.close() should cause a canceled read error #1972

Merged
merged 1 commit into from
Nov 10, 2019
Merged

fix: bindings.close() should cause a canceled read error #1972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintrc
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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