Event emitter subscription leak

[ddurschlag6river]

Error seen:
(node:96785) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 ready listeners added. Use emitter.setMaxListeners() to increase limit

Repro code:

		const c = new SftpClient();
		for (let i = 0; i < 50; i++) {
			await c.connect({
				host: '127.0.0.1',
				port: 2222,
				username: /*...*/,
				password: /*...*/,
			});
			await c.end();
		}

I might be misreading what my debugger is telling me, but it seems that https://github.com/jyu213/ssh2-sftp-client/blob/4adc77c1df4f5076c967aa973065841e1fda3abf/src/index.js#L430 is adding a .on('ready') subscription every time the client connects, but never removing them. My theory is that after repeated reconnects, the number of ready subscribers causes an exception to be thrown by EventEmitter.

[tylik1]

Any update on that issue? Experiencing too

[tylik1]

I've changed all .on to .once for the client, and that did the trick. Not sure if it may break anything else though.

Here is a link to the similar issue
mscdex/ssh2#583

[ddurschlag6river]

@tylik1 could you possibly create a pull-request for your fix? It'd be great to have it in the mainline codebase.

[tylik1]

@ddurschlag6river sorry but i'm not sure that this is a bulletproof solution. I've tested several times, but still one time i've encountered the same bug even with fix applied (

[jyu213]

connect support client method , sftp.connect(options, clientMethod),
but loop execute connect will also create multiple connections before it end

[ddurschlag6river]

@jyu213 I'm not sure I follow. Both the connect and the end are awaited, so they should their execution should alternate -- a connect, then an end, then loop and do it again.

[rkaw92]

See a related issue in the base ssh2 package:
mscdex/ssh2#717 (comment)

Basically, it seems that re-using client objects is, in general, a bad idea.

[theophilusx]

I have the same issue using the sftp.get() function. In my case, I want to pipe the chunks through gunzip to unzip the files 'on the fly'. While it is working, I keep getting the warning about possible memory leak. I do add listeners for 'finish' and 'end' (it is wrapped in a promise) and I do use 'once' rather than 'on' and I do call removeListener and still get this warning. Note that using a separate connection for each file is not really a solution as it results in the target sftp server resetting the connections (I'm assuming due to it seeing too many connectiions from the same host in a very short period). My function/promise is called via await, so never transferring more than 1 file at a time. I feel the advice to not reuse connections for multiple downloads is really just side stepping the problem. Looking at the provided links, there does not seem to be any justification for why it is a bad idea.

[arachnetech]

I'm seeing this error in version 3.1.0. Running node with --trace-warnings shows:

(node:24404) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added. Use emitter.setMaxListeners() to increase limit
at _addListener (events.js:280:19)
at Client.addListener (events.js:297:10)
at Promise (C:\Work\MDXConnect\src\node\portal\node_modules\ssh2-sftp-client\src\index.js:25:25)
at new Promise (<anonymous>)
at SftpClient.list (C:\Work\MDXConnect\src\node\portal\node_modules\ssh2-sftp-client\src\index.js:21:12)

I believe \ssh2-sftp-client\src\index.js:25 is the cause of the problem:

this.client.on('error', (err) => {

Every time list() is called, SftpClient is adding an error handler to this.client, but this handler is never removed. (See https://stackoverflow.com/questions/23893872/how-to-properly-remove-event-listeners-in-node-js-eventemitter.)

I believe every call to this.client.on('error'... (and manySftpClient functions do this - list(), get(), put(), mkdir(), rmdir(), delete(), rename(), etc.) needs to remove this event listener before returning. Otherwise if a user makes a few calls on an open SFTP connection they will encounter the MaxListenersExceededWarning.

[arachnetech]

I'm testing a fix to the two functions that I call the most (list() and get()) which removes the client error event handler before resolving/rejecting. It seems to work for me.

SftpClient-fix.js.txt

[theophilusx]

That looks interesting. I will try to find the time to test it out as well.

…

On Fri, 28 Sep 2018 at 18:13, arachnetech ***@***.***> wrote: I'm testing a fix to the two functions that I call the most (list() and get()) which removes the client error event handler before resolving/rejecting. It seems to work for me. SftpClient-fix.js.txt <https://github.com/jyu213/ssh2-sftp-client/files/2427349/SftpClient-fix.js.txt> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#78 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhUKRXcV-xhAYuyueqeCXUEbbCaZlolks5ufdoMgaJpZM4U_-lR> .

-- regards, Tim

-- Tim Cross

[theophilusx]

@arachnetech This is possibly a partial fix. However, when running the test suite, I see that the same warning is generated for the 'stat' command. This command does not add any event listeners (I'm guessing they are being added by the underlying ssh2 module). So your patch would appear to reduce the problem, but not resolve it.

EDIT: Actually, ignore this. Just realised I had missed one of the calls and it was the after hook in the mocha test that was raising the warning, not the call to stat.

[theophilusx]

I have submitted a pull request which I think fixes this issue. In looking deeper into the code, I don't think the majority of the this.client.on('error', ....) listeners are required, so removed them. All tests appear to be passing.

[arachnetech]

Thanks, that’s great!

[rhasthimuni]

Hey all,

This has been an issue that was bugging one of our implementations too. (piping files from SFTP location to a S3 bucket). I seems to have solved this issue by doing

var Client = require('ssh2-sftp-client');
...

let sftp = new Client();
...
for (o in collection){
    await someLogic()
    sftp.client.removeAllListeners('error');
}

after each of the promises have been awaited.

[theophilusx]

@rhasthimuni Yes, that would probably also work, though it can be a little dangerous to do a removeAllListeners when you are reusing the same client connection. Having said that, I don't think the original version requires all the .on error listeners that it adds. Many of the methods are just using low level ssh2 calls which encapsulate the logic and handle events, so no need to add listeners at this level - 2 possible exceptions are get/put, which return streams (though I would add listeners to the stream, not the client)..

the pull request I have submitted removes most of the on error listeners - you really just need one on error listener on the client and odn't need to add another one every time you call a new method (which is the cause of the main issue). I've been using it now for a week and have not observed the memory leak warning.

[rhasthimuni]

@rhasthimuni Yes, that would probably also work, though it can be a little dangerous to do a removeAllListeners when you are reusing the same client connection. Having said that, I don't think the original version requires all the .on error listeners that it adds. Many of the methods are just using low level ssh2 calls which encapsulate the logic and handle events, so no need to add listeners at this level - 2 possible exceptions are get/put, which return streams (though I would add listeners to the stream, not the client)..

the pull request I have submitted removes most of the on error listeners - you really just need one on error listener on the client and odn't need to add another one every time you call a new method (which is the cause of the main issue). I've been using it now for a week and have not observed the memory leak warning.

Yeah i do realize its not ideal to remove all listeners from the client, but with our implementations, it seems to be alright although it is a workaround. However I started with using the changes you have submitted (by loading your PR as a locally loaded library) and it didnt seem to solve my issue. May be because how we send the .get() stream in to a S3 .upload() via a stream.PassThrough and then resolving the upload promise. In any case i'll keep an eye out for a better solution or debug why your solution dosent seem to work for me and update this thread.

[theophilusx]

@rhasthimuni Would be very interested in any additional details you can provide re: issue with .get. We are using that method and sending the data through zlib.gunzip so that we can gunzip files 'on the fly'. We also wrap it inside a promise and it seems to be working well. Internally, it is using .pipe().

It is possible (likely?) my fix has reduced, but not yet eliminated the issue - I was somewhat conservative with changes to get/put due to the additional potential complexity wrt streams (compared to the other methods that is). I'm guessing you are 'getting' more files than we are and see the emitter warning when we don't hit the magick 10 (on average, we are retrieving 73 files that are gunzipped on the fly.

The only issue we have run into at the moment is a problem if we try to create new connections too quickly - we either get a timeout or errors about directories not existing. hard to reproduce and not sure if it is a module issue or a local network issue at this point. Solved it for now by putting a 30 second delay between new connections (we do retrieve multiple files wiht a single connection).

The other problem we have encountered is with Node v10, but this seems to be related to the underlying ssh2 module. With node 10.x, the .fastGet method hangs after downloading the first 'chunk'. I've reported this to the ssh2 maintainers, but need to generate a reproducible test case (next on my list).

[jyu213]

thanks for @rhasthimuni 's merge request. publish a new version @2.4.0 resolve it.

[rhasthimuni]

thanks for @rhasthimuni 's merge request. publish a new version @2.4.0 resolve it.

Hey.. I believe its @theophilusx 's PR.. :)

[rhasthimuni]

Ok... since @theophilusx 's merge got published (2.4.0) it seems to have fixed our issue.. ive removed

sftp.client.removeAllListeners('error');

and the memory leak warning hasnt come up yet. Will report after some observation

@theophilusx ...Thanks mate ;)

[theophilusx]

I think this issue is now resolved and can be closed