From c57b6e9ea9de08ddce0947a812c0d68e7f6f4b86 Mon Sep 17 00:00:00 2001
From: Francis Gulotta <wizard@roborooter.com>
Date: Tue, 24 Sep 2019 09:56:56 -0400
Subject: [PATCH] fix: stop polling if the poller has an error (#1936)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This will stop polling for FS events if there’s an error polling for fs events. Added tests for the JS. Tests for the c++ are harder.

This supersedes #1804 and ensures no more events are fired if one reports an error. Fixes #1803 based upon @nfrasser's work.
---
 packages/bindings/lib/poller.js      | 10 ++---
 packages/bindings/lib/poller.test.js | 60 ++++++++++++++++++++++++++++
 packages/bindings/src/poller.cpp     | 15 +++++--
 packages/bindings/src/poller.h       |  1 +
 4 files changed, 77 insertions(+), 9 deletions(-)
 create mode 100644 packages/bindings/lib/poller.test.js

diff --git a/packages/bindings/lib/poller.js b/packages/bindings/lib/poller.js
index 80e4e6e5f..085ac0231 100644
--- a/packages/bindings/lib/poller.js
+++ b/packages/bindings/lib/poller.js
@@ -1,12 +1,12 @@
 const debug = require('debug')
 const logger = debug('serialport/bindings/poller')
 const EventEmitter = require('events')
-const FDPoller = require('bindings')('bindings.node').Poller
+const PollerBindings = require('bindings')('bindings.node').Poller
 
 const EVENTS = {
-  UV_READABLE: 1,
-  UV_WRITABLE: 2,
-  UV_DISCONNECT: 4,
+  UV_READABLE: 0b0001,
+  UV_WRITABLE: 0b0010,
+  UV_DISCONNECT: 0b0100,
 }
 
 function handleEvent(error, eventFlag) {
@@ -35,7 +35,7 @@ function handleEvent(error, eventFlag) {
  * Polls unix systems for readable or writable states of a file or serialport
  */
 class Poller extends EventEmitter {
-  constructor(fd) {
+  constructor(fd, FDPoller = PollerBindings) {
     logger('Creating poller')
     super()
     this.poller = new FDPoller(fd, handleEvent.bind(this))
diff --git a/packages/bindings/lib/poller.test.js b/packages/bindings/lib/poller.test.js
new file mode 100644
index 000000000..bcdc853cb
--- /dev/null
+++ b/packages/bindings/lib/poller.test.js
@@ -0,0 +1,60 @@
+const Poller = require('./poller')
+
+class MockPollerBidnings {
+  constructor(fd, callback) {
+    this.fd = fd
+    this.callback = callback
+  }
+  poll(flag) {
+    this.lastPollFlag = flag
+    setImmediate(() => this.callback(null, flag))
+  }
+}
+
+class ErrorPollerBindings {
+  constructor(fd, callback) {
+    this.fd = fd
+    this.callback = callback
+  }
+  poll(flag) {
+    this.lastPollFlag = flag
+    setImmediate(() => this.callback(new Error('oh no!'), flag))
+  }
+}
+
+describe('Poller', () => {
+  it('constructs', () => {
+    new Poller(1, MockPollerBidnings)
+  })
+  it('can listen to the readable event', done => {
+    const poller = new Poller(1, MockPollerBidnings)
+    poller.once('readable', err => {
+      assert.equal(err, null)
+      assert.equal(poller.poller.lastPollFlag, 1)
+      done(err)
+    })
+  })
+  it('can listen to the writable event', done => {
+    const poller = new Poller(1, MockPollerBidnings)
+    poller.once('writable', err => {
+      assert.equal(err, null)
+      assert.equal(poller.poller.lastPollFlag, 2)
+      done(err)
+    })
+  })
+  it('can listen to the disconnect event', done => {
+    const poller = new Poller(1, MockPollerBidnings)
+    poller.once('disconnect', err => {
+      assert.equal(err, null)
+      assert.equal(poller.poller.lastPollFlag, 4)
+      done(err)
+    })
+  })
+  it('reports errors on callback', done => {
+    const poller = new Poller(1, ErrorPollerBindings)
+    poller.once('readable', err => {
+      assert.notEqual(err, null)
+      done()
+    })
+  })
+})
diff --git a/packages/bindings/src/poller.cpp b/packages/bindings/src/poller.cpp
index 8b4c2a881..e2d9201d5 100644
--- a/packages/bindings/src/poller.cpp
+++ b/packages/bindings/src/poller.cpp
@@ -49,22 +49,29 @@ void Poller::stop() {
   }
 }
 
+int Poller::_stop() {
+  return uv_poll_stop(poll_handle);
+}
+
 void Poller::onData(uv_poll_t* handle, int status, int events) {
   Nan::HandleScope scope;
   Poller* obj = static_cast<Poller*>(handle->data);
   v8::Local<v8::Value> argv[2];
+
+  // if Error
   if (0 != status) {
     // fprintf(stdout, "OnData Error status=%s events=%d\n", uv_strerror(status), events);
     argv[0] = v8::Exception::Error(Nan::New<v8::String>(uv_strerror(status)).ToLocalChecked());
     argv[1] = Nan::Undefined();
+    obj->_stop(); // doesn't matter if this errors
   } else {
-    // fprintf(stdout, "OnData status=%d events=%d\n", status, events);
+    // fprintf(stdout, "OnData status=%d events=%d subscribed=%d\n", status, events, obj->events);
     argv[0] = Nan::Null();
     argv[1] = Nan::New<v8::Integer>(events);
+    // remove triggered events from the poll
+    int newEvents = obj->events & ~events;
+    obj->poll(newEvents);
   }
-  // remove triggered events from the poll
-  int newEvents = obj->events & ~events;
-  obj->poll(newEvents);
 
   obj->callback.Call(2, argv, obj);
 }
diff --git a/packages/bindings/src/poller.h b/packages/bindings/src/poller.h
index 67f356916..3b15a2d4a 100644
--- a/packages/bindings/src/poller.h
+++ b/packages/bindings/src/poller.h
@@ -22,6 +22,7 @@ class Poller : public Nan::ObjectWrap, public Nan::AsyncResource {
   ~Poller();
   void poll(int events);
   void stop();
+  int _stop();
 
   static NAN_METHOD(New);
   static NAN_METHOD(poll);