6

I'm getting an error when downloading multiple files from an SFTP site using the ssh2-sftp-client library. The error thrown seems to indicate that the node stream is not getting cleared after each download completes. This is causing a memory leak in my app. In production I need to be able to download thousands of files so this memory leak is substantial. How can I close the stream so that the memory is released after each file is downloaded?

code:

const Client = require('ssh2-sftp-client');

const sftp = new Client();
sftp.connect({
  host: '195.144.107.198',
  port: 22,
  username: 'demo',
  password: 'password'
}).then(async () => {

  const fileNames = ['readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt'];

  // Loop through filenames
  for (let i = 0; i < fileNames.length; i++) {

    // Download all the files synchronously (1 at a time)
    const fileName = fileNames[i];
    await new Promise((resolve, reject) => { // <-- note the await
      sftp.get(fileName, true, 'utf8').then((stream) => {
        let text = '';
        stream
          .on('data', (d) => { text += d; })
          .on('end', () => {
            console.log('Success downloaded file', i);
            resolve(text);
          });
      }).catch((err) => {
        console.log('Error downloading file', err);
        reject(err.message)
      });
    });
  }
  sftp.end();
});

Note: this code uses a public SFTP site so the credentials are not sensitive and you can run it for testing. Found here: https://www.sftp.net/public-online-sftp-servers

Error (occurs after file #9 is downloaded):

(node:44580) MaxListenersExceededWarning: Possible EventEmitter memory leak detected.
11 error listeners added. Use emitter.setMaxListeners() to increase limit
Jon Lamb
  • 1,413
  • 2
  • 16
  • 28

1 Answers1

1

So you said that you are attempting to download thousands of files in prod but you're using a listener for each file. Node only allows you to make a max of 10 event listeners before triggering an alert.

See:

https://nodejs.org/dist/latest-v8.x/docs/api/events.html#events_eventemitter_defaultmaxlisteners https://github.com/nodejs/help/issues/1051

If you want to correct this, I'd recommend you implement a queue and only download 10 files at a time.

Something like:

const Client = require('ssh2-sftp-client');

const sftp = new Client();
sftp.connect({
  host: '195.144.107.198',
  port: 22,
  username: 'demo',
  password: 'password'
}).then(async () => {

  // Treat files array as a queue instead of an array
  const fileQueue = ['readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt', 'readme.txt'];

  // Use this function to grab files from your main files array
  const downloadFilesFromQueue = (fileName) =>
      new Promise((resolve, reject) => {

      // Sanity check
      if(!fileName) {
          resolve();
      }

      sftp.get(fileName, true, 'utf8').then((stream) => {
        let text = '';
        stream
          .on('data', (d) => { text += d; })
          .on('end', () => {
            console.log('Success downloaded file', fileName);
            resolve(text);
          });
      }).catch((err) => {
        console.log('Error downloading file', err);
        reject(err.message);
      });
    })

    // Handle errors
    .catch((err) => console.log(err.message))

    // Get next file from the queue
    .then(() => {

       // If there are no more items in the queue, we're done
       if (!fileQueue.length) {
           return;
       }

       downloadFilesFromQueue(fileQueue.shift())
    });

  // Track all unresolved promises
  const unresolvedPromises = [];

  // Request no more than 10 files at a time.
  for (let i = 0; i < 10; i++) {

    // Use file at the front of the queue
    const fileName = fileQueue.shift();

    unresolvedPromises.push(downloadFilesFromQueue(fileName));
  }

  // Wait until the queue is emptied and all file retrieval promises are 
  // resolved.
  await Promise.all(unresolvedPromises);

  // done
  sftp.end();
});
Mike
  • 3,830
  • 2
  • 16
  • 23
  • 1
    Two questions for you. One is that I tried running your code and it appears to go into an infinite loop inside the recursive function and throws a whole bunch of errors. Two, won't I still run into the max listener problem even with a queing system? In my OP I essentially had a queing system of 1, and it should have waited for each 1 to download before starting the second 1. I feel like even with a que of 10, once I get to the 11th in the second que I will get the max listener error. – Jon Lamb Oct 16 '18 at 02:00
  • I just took a look over it and realized there was a missing sanity check (if the string is empty it'd likely trigger errors). Also added another check to make sure that the recursive function doesn't send empty strings to be processed. As to your second question I missed the `await` before your promise so I thought it was spawning several listeners in parallel but I can't confirm one way of the other if that is the case. So you're right it is the same logic. Unless calls to `sftp.get` spawn new listeners that aren't torn down until `sftp.end()` is called I'm not sure what's causing this. – Mike Oct 16 '18 at 03:17