ssh2 icon indicating copy to clipboard operation
ssh2 copied to clipboard

Issue/Suggestion :- Adding of maxEventListner on all the event

Open abhishekglb opened this issue 1 year ago • 4 comments

Hello,

I am currently using a Node.js package (https://www.npmjs.com/package/ssh2-sftp-client) that utilizes another package (https://www.npmjs.com/package/ssh2).

Recently, I have been encountering the following memory leak error on my production environment: "Possible Event Emitter memory leak detected. 10 close/end listeners added to [Client]. Use setMaxEventListener to increase the limit."

This error occurs frequently when multiple SFTP users are accessing the system. I was wondering if it's possible to add "_this.setMaxListeners(0);" to address this issue.

Thank you for your assistance. Please let me know if this update can be implemented.

abhishekglb avatar Mar 01 '24 13:03 abhishekglb

Can you reproduce the issue using only ssh2 and using a minimal example? That should rule out any downstream issues or issues with the code utilizing ssh2/ssh2-sftp-client.

mscdex avatar Mar 01 '24 17:03 mscdex

I am the maintainer of ssh2-sftp-client.

I would not agree with adding that setting as all it will do is disable the warning. The warning fulfils an important role in alerting developers to possible code errors/problems. Note that the warning is not saying there is a memory leak, but rather there is an indicator of a possible source for a memory leak. Disabling this warning merely covers up an issue which is likely going to be the source of other problems. First step is to identify and understand the cause trigering this warning.

In most use cases, you should not be seeing this warning. It indicates either;

  • You are not closing sftp connections correctly. Fix is to find where your code is leaving connections open and adjust appropriately

  • You are using an outdated version of ssh2-sftp-client. Managing listgeners within a promise based API wrapped around an event driven API is challenging. There have been issues in the past where edge cases have existed that would result in event listeners not being removed when they should (or being added when not necessary). No such issues are known in the current version. Fix is to ensure your running the latest version.

  • An edge case still exists where either additional listeners are being added when they should not be or are note being removed after the connection is closed. This needs to be fixed, but to do that, I would need some sample code with reproduces the issue. I would then use this code to both fix the issue and as the basis for additional unit tests to protect against regressions in future releases.

  • You have an extremely unusual use case which results in this warning. No known use case of this tyhpe has been identified, so this is very unlikely. However, if you feel this is the case, please let me know as there may be a better way the module can manage that use case.

First step would be to confirm you are running the latest version of ssh2-sftp-client and you are running it on a supported version of node. AFter that, verify that all your code paths are terminated correctly i.e. by calling the end() method. If the problem still exists, try and create a minimal script which reproduces the error and report an issues against the ssh2-sftp-client project so that it can be investigated.

theophilusx avatar Mar 01 '24 21:03 theophilusx

So, my current use case is as follows: I have multiple SFTP connections per user, and multiple users try to upload files from their configured SFTP. So, I have created an SFTP object like this:

let sftp_user = {};

sftp_user[user_id] = new SftpClient();

// Your file upload process here

delete sftp_user[user_id];

In this setup, the file upload process starts by verifying the FileSystemDirectory, and once the file is successfully uploaded, then connection is closed.

abhishekglb avatar Mar 04 '24 05:03 abhishekglb

If you are still working at the ssh2-sftp-client level and not using just plain ssh2, please log this issue against the ssh2-sftp-client repository at github.com/theophilusx/ssh2-sftp-client. If/when you do, please also include the version of ssh2-sftp-client you are using and the version of node you are running on.

Based on the information provided, I don't think there is anything obvious which would cause the warning. You are creating separate ssh2-sftp-client objects per user and node counts the listeners on a per emitter basis i.e. each innstance will have its own count. There are a couple of possible causes which I can think of

Is each user uploading multiple files and are you creating new connections for each upload using the same ssh2-sftp-client object?

If you are, try to avoid this. You can perform multiple operations using the same object and it will be a lot faster. Establishing sftp connections is a slow process and you only want to do it as few times as possible.

I'm suspicious that the end() call may not be remvoing all the global listeners and this may be the cause. However, I would need to investigate that to verify.

If you are creating multiple conections, try modifying your code so that only a single connection is created and all the uploads/download/verifying is done over that one connection. If you cannot do that, perhaps change your code so that it creates a new sftp-client-object for each connection instead of trying to reuse and existing object (there are some other reasons this mayh also be a better idea).

theophilusx avatar Mar 04 '24 06:03 theophilusx