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

Can't release EventEmitter hooked to fs.ReadStream (Memory leak) #3289

Closed
7h0mas-R opened this issue Mar 26, 2021 · 6 comments
Closed

Can't release EventEmitter hooked to fs.ReadStream (Memory leak) #3289

7h0mas-R opened this issue Mar 26, 2021 · 6 comments

Comments

@7h0mas-R
Copy link

7h0mas-R commented Mar 26, 2021

  • Node.js Version: 8.11.1 (old but restricted by the system I am on)
  • OS: volumio2.873 (Raspbian Jessie variant)
  • Scope (install, code, runtime, meta, other?): code
  • Module (and version) (if relevant): fs

I read data from a dtoverlay device. Reading works fast and reliable, but I cannot properly clean up resoureces when the script ends:

  • I observe, that my test-scripts do not properly close without hitting Ctrl-C.
  • When I try to remove the dynamic overlay with sudo dtoverlay -r I get a Kernel warning about a potential memory leak.

I tracked it down to a problem in an npm module I use (input-event). There seems to be a FSREQWRAP handle remaining somewhere, that causes both issues. I stripped down the module to the base and have the exact line that causes the fail. It seems to be related to an EventListener attached to an fs.Readstream object.
Below is the stripped down code snippet with the line causing the fail highlighted. Removing the listener is not helping.
Any ideas? Is this a bug or is the implementation wrong? Despite experience in other languages, I am new to JS and really have difficulties spotting the issue here.

const log = require('why-is-node-running');  //only used to list out handles blocking the end of the script
const fs = require('fs');
const EventEmitter = require('events');

class InputEvent extends EventEmitter {
  constructor(device, options) {
    super();
    Object.assign(this, {
      device,
      flags: 'r',
      encoding: null
    }, options);
    this.on('raw', data => console.log(data));
    this.fd = fs.openSync(this.device, this.flags);
    this.input = fs.createReadStream(null, this);
    this.input.on('data', data => console.log(data));  //##### when this line is added, the the orphan handle is created
    console.log('Listeners #:',this.input.listenerCount('data'));  // >>> 1  
    this.input.removeAllListeners();                  // even removing the listener again does not help
    console.log('Listeners #:',this.input.listenerCount('data'));  // >>> 0
    this.input.on('error', err => {
        console.log(err);
    });
  }

  close(callback) {
    this.fd.close(callback);
    return this;
  }
}

myEvent = new InputEvent('/dev/input/event0');


setTimeout(()=>{
    log();
  },2000)

The why-is-node-running module reports 2 unreleased handles (one of them is from the timeOut I use to trigger why-is-node-runningbut the other is generated by the line indicated above.

There are 2 handle(s) keeping the process running
# Timeout
/data/INTERNAL/test/index_debug.js:34 - setTimeout(()=>{
# FSREQWRAP
(unknown stack trace)

I'm at a loss - ideas welcome...

@7h0mas-R 7h0mas-R changed the title Trying to hook up an EventEmitter to an input device stream Can't release EventEmitter hooked to fs.ReadStream (Memory leak) Mar 27, 2021
@Ayase-252
Copy link
Member

I don't familiar with lib you used here.

Just an random idea, could this be related to that this.fd? Since I do not see anyway for close to be called, therefore this.fd may be leaked.

@7h0mas-R
Copy link
Author

Thank you for the reply and suggestion.
I was also wondering about that, since the way I see it, the input stream from the device will never end by itself, so close will probably not be called (the way I understand the docs, close get's normally called, when the stream ends).
So it could be, that the file handle does not get released, as long as the stream is not at a kind of EOF.
But I did not manage to artificially end it by anything I tried.
Maybe I'll try to further simplify the script and try a different implementation without the module.

@addaleax
Copy link
Member

fs.createReadStream() returns a stream that uses blocking reads (although on a different thread) to read from the underlying file. These reads won’t complete in this case, leaving your process hanging. You may want to prefer using a net.Socket() instance if you want to read from non-blocking fds.

@7h0mas-R
Copy link
Author

thanks for the hint - stumbled over this difference in the node.js doc, but could not make sense of it yet - as mentioned, I'm rather new to js.
Will look into this and try to implement a solution with net.Socket(), other than the npm module I used so far. Although it is probably no issue in my application, I do not like the idea of potential memory leaks.
Close this for now and will add a snippet with my solution.

@7h0mas-R 7h0mas-R reopened this Mar 29, 2021
@7h0mas-R
Copy link
Author

As promised, short update:
I spent the last 2 evenings googling and digging through documentations - but finally gave up on this. Seems, like others have tried before and failed, too. Since as mentioned before I am new to nodejs, this is beyond my capabilities.

I tried two things:

  1. Pass the path to my input device directly to net.socket() like so

    const sock = new net.Socket({
    fd:  '/dev/input/event0/',
    readable: true,
    writable: false
    })
    

    This give me a TypeError: Unsupported fd type: TTY(actually, it is an input device, so rather a keyboard than a display)

  2. Obtain an FD using fs.open and pass that to net.Socket(), like so

    const fd = fs.openSync('/dev/input/event0', 'r');
    const input = new net.Socket({fd:fd,readable:true})
    

    but that earns me a TypeError: Unsupported fd type: FILE

A similar issue seems to be described here:
nodejs/node#15439

I am at a loss here, but as mentioned earlier, in my case the FD is only opened at boot and closed at shutdown once, so the risk is limited to see any issue. However, if a user would do a lot of configuration changes in one session, it would probably create issues, since the FD would be (improperly) closed and reopened at every reconfig. So not nice but low risk.
Suggestions or examples how it can still be done properly are still welcome.

@7h0mas-R
Copy link
Author

I looked into this more and still could not get it to work. But I found a workaround using the hint that @kuba-orlik gave in nodejs/node#15439:

Instead of using the combination of openSync + createReadStreamcombination, use

spawn = require("child-process").spawn;

...

this.fd = spawn("cat", ["/dev/input/event0"]);
this.fd.stdout.on("data", on('data', data => console.log(data));

to create the handle. You can then later kill the resource by calling

this.fd.kill();

In my case this works and seems to solve my problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants