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.

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

support for polling linux character device for incoming data #9285

Closed
mbroadst opened this issue Oct 25, 2016 · 21 comments · May be fixed by adamlaska/node#99
Closed

support for polling linux character device for incoming data #9285

mbroadst opened this issue Oct 25, 2016 · 21 comments · May be fixed by adamlaska/node#99
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@mbroadst
Copy link

mbroadst commented Oct 25, 2016

Hi, I'm trying to determine whether it's possible to poll a linux character device using node.js, and where exactly the breakdown occurs between libuv and node. I'm putting together a small experimental qemu agent which currently requires communicating over a virtio-serial provided character device, and as far as I have tried it doesn't seem possible presently to monitor that fd through any means provided to me in node.

As a first step I just wanted to check if it might be a limitation of epoll, so I took a modified version of this code, compiled it and listened to the device for incoming data, and was able to verify that epoll in fact notified on change.

I've tried the following methods:

  • fs.watch - this doesn't throw any errors, but simply never emits a change event on incoming data.
  • net.createConnection - this throws a ECONNREFUSED as expected, since this is not a socket
  • using tty.ReadStream - also fails because of course this isn't a TTY

My only remaining option has been to simply poll every Nms using fs.read or fs.readFile within a class duck typed as a net.Socket, which is less than ideal. I thought I might just get away with the fs.watch approach, but from perusing the source code it looks like that is bound to a uv_fs_event_start which is probably falling back to inotify in this case.

So three questions occur to me at this point:

  • am I completely botching this and missing some obvious API
  • would there be interest in supporting such a feature (I'm more than willing to implement this)
    • if there is, where would such functionality live? fs, net, os, something else?

Thanks!

@mscdex mscdex added the feature request Issues that request new features to be added to Node.js. label Oct 26, 2016
@mscdex
Copy link
Contributor

mscdex commented Oct 26, 2016

Does fs.createReadStream() not work?

@mbroadst
Copy link
Author

@mscdex fs.createReadStream works but immediately ends once it reads all the data. My problem isn't with reading data from the character device per se, rather determining when data arrives in an asynchronous manner.

@mscdex
Copy link
Contributor

mscdex commented Oct 26, 2016

@mbroadst What do you mean by "once it reads all the data?" I was thinking the 'data'/'readable' events would be your clue as to when data has arrived.

@mbroadst
Copy link
Author

@mscdex ah, yeah sorry I should be more clear. Given the following code snippet:

const fs = require('fs');
let stream = fs.createReadStream('/dev/virtio-ports/test.agent');
stream.on('data', d => console.log('rx: ', d));

If there is no data currently available in the device the script immediately exits. If there is data it consumes the data, prints it, and exits again. It does not continue to watch the file for further changes. (I actually used that stream's implementation to model my own which continuously retries the fs.read on the device)

@mscdex
Copy link
Contributor

mscdex commented Oct 26, 2016

So I asume you see an 'end' event on the stream before the process exits?

@mbroadst
Copy link
Author

mbroadst commented Oct 26, 2016

Yes, however that doesn't help all that much if you were perhaps thinking of using that to kick off a new stream - I basically end up with the same solution of constantly polling the device, rather than acting upon a notification that new data is available (something epoll will do for me)

@mscdex
Copy link
Contributor

mscdex commented Oct 26, 2016

Does the serialport module work in this case?

@mbroadst
Copy link
Author

mbroadst commented Oct 26, 2016

@mscdex hm yes surprisingly it does seem to work, though I wouldn't have expected it to because this isn't in fact a serial port. In fact, all that would be needed would be their serialport_poller it looks like.

I've read a number of the threads about the serialport module, and lack of interest in including any of it in node's core code, but perhaps there would be less resistance to a more generalized means of interacting with libuv's polling facilities?

Whoa, not really sure where I got the idea that there was resistance 😄 Rather it looks like work is needed in libuv to provide the ioctl support needed. I still wonder if there might be interest in getting the generic access to polling.

@sam-github
Copy link
Contributor

Serial ports are one of the original character devices.

I think there is a way to do this, by opening a fd on the device with fs.open, then creating a net.socket using that fd, which may be undocumented, I can't recall how to do it off the top of my head, but its necessary when fds are passed across ipc channels, there is a way. cdevs look more like sockets than like files, which is why the fs stream things don't work.

The serialport module would probably be more robust.

@mbroadst
Copy link
Author

mbroadst commented Oct 26, 2016

@sam-github right. However, while serial ports are character devices, not all character devices are serial ports. I forgot to mention that I had indeed tried your approach as well to no avail:

let fd = fs.openSync('/dev/virtio-ports/test.agent', 'r');
let socket = new net.Socket({ fd: fd });
socket.on('data', d => console.log('rx: ', d));

results in:

net.js:35
  throw new TypeError('Unsupported fd type: ' + type);
  ^

TypeError: Unsupported fd type: FILE
    at createHandle (net.js:35:9)
    at new Socket (net.js:144:20)
    at Object.<anonymous> (/root/test.js:17:14)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:394:7)

There doesn't appear to be any way to tap into this from the user side either, as the handle type is coming from TTYWrap and the two other wraps (Pipe and Tcp) aren't exported. This would be a ripe spot, I agree, to throw in maybe a CharacterDeviceWrap or something similar - though I think it might be more flexible to have a generic method of interfacing with libuv's polling capabilities. At very least this would already be useful as a replacement for the serialport module's existing poller.

I don't necessarily agree that the serialport module would be more robust, as it's wildly overweight for my particular use case (I'm clocking in at ~11M right now for node_modules, just for an npm install serialport).

@bnoordhuis
Copy link
Member

If you are comfortable using unstable and undocumented APIs, you can create the handle like this:

const fd = /* ... */;
const Pipe = process.binding('pipe_wrap').Pipe;
const handle = new Pipe();
handle.open(fd);
const socket = new net.Socket({ handle });

Note that passing a file descriptor that is not compatible with epoll will almost certainly abort the process. Everything from process.binding is unstable by definition so use at your own peril.

it might be more flexible to have a generic method of interfacing with libuv's polling capabilities

There is nothing stopping you from writing a small C++ add-on that does that but as I mentioned, libuv will simply abort when the file descriptor is incompatible. That's why createHandle() restricts it to just pipe and TCP file descriptors.

@sam-github
Copy link
Contributor

I don't necessarily agree that the serialport module would be more robust, as it's wildly overweight for my particular use case (I'm clocking in at ~11M right now for node_modules, just for an npm install serialport).

a) I've never understood the tendency to avoid using a library because it has too many features and is "too mature", so unless you literally are on a resource constrained system and can't afford the disk space, that 11 meg is irrelevant. Except in this case...
b) 7+ meg of that is node-pre-gyp, which mostly exists to support windows users by downloading precompiled binaries for them, and has zero run-time cost, its just run by the npm install script. I wouldn't say using node-pre-gyp makes a node addon "overweight", but that's your call, and its trivially strippable from the serialport module if you fork it, or commit it as a dep.

@mbroadst
Copy link
Author

@bnoordhuis ah, I didn't know I had access to the internal bindings that way - thank you, I'll give that a shot.

@sam-github I should clarify that I'm not opposed to using the serialport module, rather that I see at least two use cases here for direct access to libuv's epoll interface, and paired with what appears to be a node.js tradition of generally providing a "binding to libuv" I wanted to discuss furthering that mission (so to speak). The resource constraint is just a supplemental argument, but I believe has merit in that the proposal here is to import ~14 modules, plus an optional compilation step, to solve a case that node already essentially satisfies.

I have no issues creating my own custom module to do this in a far more compact fashion than serialport (for my case), I just wanted to see if there would be any interest in providing a more standardized, and reusable addition to node for the future considering the code added would be minimal.

@mbroadst
Copy link
Author

mbroadst commented Oct 26, 2016

@bnoordhuis it doesn't look like PipeWrap will work since it emits a UV_EOF when it completes a read from the character device, and net.Socket will end at that point. PipeWrap also uses the uv_pipe_ API which might be at odds with this type of situation?

update: as far as I can tell grepping through the source, none of the wraps provide direct access to uv_poll_init'/uv_poll_start/uv_poll_stop` on an fd

@bnoordhuis
Copy link
Member

It sounds like what you need is a small add-on that wraps uv_poll_t. It shouldn't be hard to write and I expect one or more already exist.

@mbroadst
Copy link
Author

@bnoordhuis yes that's correct, in fact the purpose of this issue was to determine if adding such functionality to node directly would be welcomed

@sam-github
Copy link
Contributor

I don't necessarily agree that the serialport module would be more robust, as it's wildly overweight for my particular use case (I'm clocking in at ~11M right now for node_modules, just for an npm install serialport).

a) I've never understood the tendency to avoid using a library because it has more than the exact feature set need at this moment, so unless you literally are on a resource constrained system and can't afford the disk space, that 11 meg is irrelevant, and also easily removable.

b) 7+ meg of that is node-pre-gyp, which mostly exists to support windows users by downloading precompiled binaries for them, and has zero run-time cost, its just run by the npm install script before building the addon. I wouldn't say using node-pre-gyp makes a node addon "overweight", but that's your call, and its trivially strippable from the serialport module if you fork it, or commit serialport as a dep ("vendor" it).

@mbroadst
Copy link
Author

@sam-github are you just trying to reiterate your point here, or was that accidentally sent again?

@sam-github
Copy link
Contributor

That was definitely an accident! Don't know what happened there, it looked unsent, sorry.

@tbarusseau
Copy link

Has there been any update on this feature request? I've done some research and it looks like the situation hasn't changed.

@Trott
Copy link
Member

Trott commented Aug 2, 2017

Seems to me like the consensus of the project Collaborators involved in this discussion is that this is better suited for an add-on/third-party module than for core. I'm going to close this at this time. If I've misunderstood or if you otherwise believe that closing this is the wrong thing to do, please comment or (if GitHub allows you to do so) re-open. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants