sanketbajoria / ssh2-promise

ssh with promise/async await and typescript support
https://www.npmjs.com/package/ssh2-promise
MIT License
148 stars 24 forks source link

Event leak when cache is disabled #56

Closed jbabuscio closed 3 years ago

jbabuscio commented 3 years ago

I started receiving Node warnings that multiple listeners were being added to events and growing, specifically for [tunnel, ssh:continue, ssh:disconnect, and ssh].

After debugging, it appears that when caching is enabled, the listeners are repeatedly added because this isRegistered check always returns false since "this.deregister" is always an empty array since I'm calling "new SSH2Promise(config);" on each new configuration.

This line: https://github.com/sanketbajoria/ssh2-promise/blob/930f5f20217c052a50e2e4ade74edcf38d12d4f7/src/index.ts#L156

Quickly updating to this seems to resolve the issue... if (!isRegistered(ret, ret && ret.sshTunnels && ret.sshTunnels.length > 0 ? ret.__sshTunnels[0] : this )) {...

There seems to be a state issue where the cache is static and overlaps objects.

Note: if I disable caching, it all seems to work fine for my use case.

sanketbajoria commented 3 years ago

@jbabuscio Actually this is expected behavior, if you are creating new SSH2Promise(config) everytime. Then listeners will be attached everytime. To remove listeners, once you are done with SSH2Promise Object just close it, it will deregister all listener.

sanketbajoria commented 3 years ago

@jbabuscio Please refer to this spec, if you wanted to understand more.

 it("should check the cache and listeners attached to server, with multiple tunnels, after connect and after disconnect", function (done) {
        var sshTunnel = new SSHTunnel(sshConfigs.multiple);
        var sshTunnel2 = new SSHTunnel(sshConfigs.multiple)
        sshTunnel.connect().then((ssh) => {
            expect(ssh).toBeDefined();
            expect(Object.keys(SSHTunnel.__cache).length).toBe(2);
            Object.keys(SSHTunnel.__cache).forEach((k) => {
                expect(SSHTunnel.__cache[k].listenerCount(SSHConstants.CHANNEL.SSH)).toBe(1);
                expect(SSHTunnel.__cache[k].listenerCount(SSHConstants.CHANNEL.TUNNEL)).toBe(1);
                expect(SSHTunnel.__cache[k].listenerCount(`${SSHConstants.CHANNEL.SSH}:${SSHConstants.STATUS.DISCONNECT}`)).toBe(1);
            });
            return sshTunnel2.connect();
        }, (error) => {
            expect(error).toBeUndefined();
        }).then((ssh) => {
            expect(ssh).toBeDefined();
            expect(Object.keys(SSHTunnel.__cache).length).toBe(2);
            Object.keys(SSHTunnel.__cache).forEach((k) => {
                expect(SSHTunnel.__cache[k].listenerCount(SSHConstants.CHANNEL.SSH)).toBe(2);
                expect(SSHTunnel.__cache[k].listenerCount(SSHConstants.CHANNEL.TUNNEL)).toBe(2);
                expect(SSHTunnel.__cache[k].listenerCount(`${SSHConstants.CHANNEL.SSH}:${SSHConstants.STATUS.DISCONNECT}`)).toBe(2);
            });
            return sshTunnel.close();
        }, (error) => {
            expect(error).toBeUndefined();
        }).then(() => {
            expect(Object.keys(SSHTunnel.__cache).length).toBe(2);
            Object.keys(SSHTunnel.__cache).forEach((k) => {
                expect(SSHTunnel.__cache[k].listenerCount(SSHConstants.CHANNEL.SSH)).toBe(1);
                expect(SSHTunnel.__cache[k].listenerCount(SSHConstants.CHANNEL.TUNNEL)).toBe(1);
                expect(SSHTunnel.__cache[k].listenerCount(`${SSHConstants.CHANNEL.SSH}:${SSHConstants.STATUS.DISCONNECT}`)).toBe(1);
            });
            return sshTunnel2.close();
        }).finally(() => {
            expect(Object.keys(SSHTunnel.__cache).length).toBe(0);
            Object.keys(SSHTunnel.__cache).forEach((k) => {
                expect(SSHTunnel.__cache[k].listenerCount(SSHConstants.CHANNEL.SSH)).toBe(0);
                expect(SSHTunnel.__cache[k].listenerCount(SSHConstants.CHANNEL.TUNNEL)).toBe(0);
                expect(SSHTunnel.__cache[k].listenerCount(`${SSHConstants.CHANNEL.SSH}:${SSHConstants.STATUS.DISCONNECT}`)).toBe(0);
                expect(SSHTunnel.__cache[k].listenerCount(`${SSHConstants.CHANNEL.SSH}:${SSHConstants.STATUS.CONTINUE}`)).toBe(0);
            });
            done();
        });
    }); 
sanketbajoria commented 3 years ago

Marking this as stale issue