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

@@ -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')

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) {

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)
           })
         })
       })

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) {

packages/serialport/test/integration.js → 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) {

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')

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', () => {