From 3a132792e7021035757458c04479973b7516e2f5 Mon Sep 17 00:00:00 2001 From: Francis Gulotta Date: Fri, 30 Aug 2019 20:07:58 -0400 Subject: [PATCH] fix: stream read not working past 1 read (#1925) This was overlooked in our async function overhaul and now has better testing. --- packages/binding-abstract/binding-abstract.js | 8 ++++---- packages/binding-mock/binding-mock.js | 6 +++--- packages/bindings/lib/bindings.test.js | 5 +++-- packages/bindings/lib/win32.js | 7 +++++-- .../integration.js => lib/serialport-test.js} | 2 +- packages/stream/stream.js | 6 +++--- packages/stream/stream.test.js | 19 ++++++++++++++++--- 7 files changed, 35 insertions(+), 18 deletions(-) rename packages/serialport/{test/integration.js => lib/serialport-test.js} (99%) diff --git a/packages/binding-abstract/binding-abstract.js b/packages/binding-abstract/binding-abstract.js index ac1d64ae6..580e78e02 100644 --- a/packages/binding-abstract/binding-abstract.js +++ b/packages/binding-abstract/binding-abstract.js @@ -85,12 +85,12 @@ The in progress reads must error when the port is closed with an error object th throw new TypeError('"buffer" is not a Buffer') } - if (typeof offset !== 'number') { - throw new TypeError('"offset" is not an integer') + if (typeof offset !== 'number' || isNaN(length)) { + throw new TypeError(`"offset" is not an integer got "${isNaN(length) ? 'NaN' : typeof offset}"`) } - if (typeof length !== 'number') { - throw new TypeError('"length" is not an integer') + if (typeof length !== 'number' || isNaN(length)) { + throw new TypeError(`"length" is not an integer got "${isNaN(length) ? 'NaN' : typeof length}"`) } debug('read') diff --git a/packages/binding-mock/binding-mock.js b/packages/binding-mock/binding-mock.js index 6104ef8b6..f02348e9a 100644 --- a/packages/binding-mock/binding-mock.js +++ b/packages/binding-mock/binding-mock.js @@ -149,10 +149,10 @@ class MockBinding extends AbstractBinding { }) } const data = this.port.data.slice(0, length) - const readLength = data.copy(buffer, offset) + const bytesRead = data.copy(buffer, offset) this.port.data = this.port.data.slice(length) - debug(this.serialNumber, 'read', readLength, 'bytes') - return readLength + debug(this.serialNumber, 'read', bytesRead, 'bytes') + return { bytesRead, buffer } } async write(buffer) { diff --git a/packages/bindings/lib/bindings.test.js b/packages/bindings/lib/bindings.test.js index ddd4d3b23..bb2d4ccb1 100644 --- a/packages/bindings/lib/bindings.test.js +++ b/packages/bindings/lib/bindings.test.js @@ -544,9 +544,10 @@ function testBinding(bindingName, Binding, testPort) { return binding.read(buffer, 0, buffer.length) }) - it('returns at maximum the requested number of bytes', () => { - return binding.read(buffer, 0, 1).then(bytesRead => { + it('returns at maximum the requested number of bytes and the buffer', () => { + return binding.read(buffer, 0, 1).then(({ bytesRead, buffer: returnedBuffer }) => { assert.equal(bytesRead, 1) + assert.equal(buffer, returnedBuffer) }) }) }) diff --git a/packages/bindings/lib/win32.js b/packages/bindings/lib/win32.js index 6c5b2d0fb..cc12b04b1 100644 --- a/packages/bindings/lib/win32.js +++ b/packages/bindings/lib/win32.js @@ -65,12 +65,15 @@ class WindowsBinding extends AbstractBinding { async read(buffer, offset, length) { await super.read(buffer, offset, length) - return asyncRead(this.fd, buffer, offset, length).catch(err => { + try { + const bytesRead = await asyncRead(this.fd, buffer, offset, length) + return { bytesRead, buffer } + } catch (err) { if (!this.isOpen) { err.canceled = true } throw err - }) + } } async write(buffer) { diff --git a/packages/serialport/test/integration.js b/packages/serialport/lib/serialport-test.js similarity index 99% rename from packages/serialport/test/integration.js rename to packages/serialport/lib/serialport-test.js index 3b934b6eb..1051131ea 100644 --- a/packages/serialport/test/integration.js +++ b/packages/serialport/lib/serialport-test.js @@ -1,5 +1,5 @@ const crypto = require('crypto') -const SerialPort = require('../') +const SerialPort = require('.') let platform switch (process.platform) { diff --git a/packages/stream/stream.js b/packages/stream/stream.js index 28bc33fef..d3c8f9734 100644 --- a/packages/stream/stream.js +++ b/packages/stream/stream.js @@ -370,10 +370,10 @@ SerialPort.prototype._read = function(bytesToRead) { const start = pool.used // the actual read. - debug('_read', `reading`) + debug('_read', `reading`, { start, toRead }) this.binding.read(pool, start, toRead).then( - bytesRead => { - debug('binding.read', `finished`) + ({ bytesRead }) => { + debug('binding.read', `finished`, { bytesRead }) // zero bytes means read means we've hit EOF? Maybe this should be an error if (bytesRead === 0) { debug('binding.read', 'Zero bytes read closing readable stream') diff --git a/packages/stream/stream.test.js b/packages/stream/stream.test.js index d450b7d8b..1ef75f08a 100644 --- a/packages/stream/stream.test.js +++ b/packages/stream/stream.test.js @@ -458,7 +458,7 @@ describe('SerialPort', () => { }) }) - it('emits the close event and runs the callback', done => { + it('emits the "close" event and runs the callback', done => { let called = 0 const doneIfTwice = function() { called++ @@ -472,7 +472,7 @@ describe('SerialPort', () => { port.on('close', doneIfTwice) }) - it('emits an error event or error callback but not both', done => { + it('emits an "error" event or error callback but not both', done => { const port = new SerialPort('/dev/exists', { autoOpen: false }) let called = 0 const doneIfTwice = function(err) { @@ -487,7 +487,7 @@ describe('SerialPort', () => { port.close(doneIfTwice) }) - it('fires a close event after being reopened', done => { + it('emits a "close" event after being reopened', done => { const port = new SerialPort('/dev/exists', () => { const closeSpy = sandbox.spy() port.on('close', closeSpy) @@ -894,6 +894,19 @@ describe('SerialPort', () => { port.binding.write(testData) }) }) + + it('emits data events with resuming', async () => { + const testData = Buffer.from('I am a really short string') + const port = new SerialPort('/dev/exists', { bindingOptions: { echo: true } }) + await new Promise(resolve => port.on('open', resolve)) + await new Promise(resolve => port.write(testData, resolve)) + await new Promise(resolve => port.once('readable', resolve)) + const data1 = port.read() + await new Promise(resolve => port.write(testData, resolve)) + await new Promise(resolve => port.once('readable', resolve)) + const data2 = port.read() + assert.deepEqual(Buffer.concat([data1, data2]), Buffer.concat([testData, testData])) + }) }) describe('disconnect close errors', () => {