Should the Poller start polling the file handle again if it encounters an error?

[nfrasser]

Summary of Problem

I'm trying to connect to a USB device on macOS. I can do so successfully and read data. However, when I manually disconnect the USB cable, the process sometimes gets stuck: CPU usage jumps to 100% and I have to forcefully kill it with a task manager.

The issue is quite rare (happens less than 5% of the time) but is very disruptive as I'm not using an environment where a force-restart of the node process is viable.

I stepped through the bindings C++ code and found the issue here: https://github.com/node-serialport/node-serialport/blob/a80d0b73a1d8424901ab73b314de344dad798d12/packages/bindings/src/poller.cpp#L52-L70

Specifically, I keep seeing status == -9, which is libuv's UV_EBADF "bad file descriptor" error. Line 67 calls the poller again regardless of whether there was a valid event or an error code. onData is called again with the exact same error status very shortly afterwards.

Code to Reproduce the Issue

const serialport = new SerialPort(path)
const parser = serialport.pipe(new SerialPort.parsers.Readline)
parser.on('data', (data) => console.log(data))

// manually disconnect the USB cable

Versions, Operating System and Hardware

• SerialPort@7.1.4
• Node.js v10.15.1
• Mac
• MacBook Pro (Retina, Mid 2012) 2.6 GHz Intel Core i7

[nfrasser]

My suggestion is to move line 67 and 68 into the else case so that polling is restarted only when no error occurs.

diff --git a/packages/bindings/src/poller.cpp b/packages/bindings/src/poller.cpp
index 8b4c2a8..d611138 100644
--- a/packages/bindings/src/poller.cpp
+++ b/packages/bindings/src/poller.cpp
@@ -61,10 +61,10 @@ void Poller::onData(uv_poll_t* handle, int status, int events) {
     // fprintf(stdout, "OnData status=%d events=%d\n", status, 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);
 }

Any pitfalls with this? If not I'm happy to make a pull request.

[reconbot]

This sounds right to me, please do!

[nfrasser]

Done, #1804

[reconbot]

Thanks for the heads up! I couldn't get this to error after pulling the plug a lot of times on my test rig. However if we do get into this bad state #1936 should take care of it. Do let me know if this helps. Published in 8.0.3 the latest beta.

Successfully published:
 - @serialport/bindings@8.0.2
 - @serialport/list@8.0.2
 - @serialport/repl@8.0.2
 - serialport@8.0.2
 - @serialport/stream@8.0.2
 - @serialport/terminal@8.0.2