Fix timeout on RETR with TLS

[ldm314]

The issue looks to be either a race condition or sequencing error. I changed the RETR command to send the 150 response after opening the file, but before waiting for the socket to be connected. This resolved this issue with both WinSCP and an old hardware device I am working with.

[matt-forster]

@ldm314 I am still planning on getting this merged in, I will have some time next week to address this.

[matt-forster]

We had this.fs.read wrapped in a Promise.try block previously because it isn't necessarily going to return a promise in every implementation - best to include that, and your tests will begin passing again most likely.

[ideadesignmedia]

Found solution, replace src/commands/registration/retr.js:

const Promise = require('bluebird');

module.exports = {
  directive: 'RETR',
  handler: function ({log, command} = {}) {
    if (!this.fs) return this.reply(550, 'File system not instantiated');
    if (!this.fs.read) return this.reply(402, 'Not supported by file system');

    const filePath = command.arg;
this.reply(150).then(() => this.connector.socket && this.connector.socket.resume())
    return this.connector.waitForConnection()
    .tap(() => this.commandSocket.pause())
    .then(() => Promise.try(() => this.fs.read(filePath, {start: this.restByteCount})))
    .then((fsResponse) => {
      let {stream, clientPath} = fsResponse;
      if (!stream && !clientPath) {
        stream = fsResponse;
        clientPath = filePath;
      }
      const serverPath = stream.path || filePath;

      const destroyConnection = (connection, reject) => (err) => {
        if (connection) connection.destroy(err);
        reject(err);
      };

      const eventsPromise = new Promise((resolve, reject) => {
        stream.on('data', (data) => {
          if (stream) stream.pause();
          if (this.connector.socket) {
            this.connector.socket.write(data, () => stream && stream.resume());
          }
        });
        stream.once('end', () => resolve());
        stream.once('error', destroyConnection(this.connector.socket, reject));

        this.connector.socket.once('error', destroyConnection(stream, reject));
      });

      this.restByteCount = 0;

      return eventsPromise.tap(() => this.emit('RETR', null, serverPath))
      .then(() => this.reply(226, clientPath))
      .finally(() => stream.destroy && stream.destroy());
    })
    .catch(Promise.TimeoutError, (err) => {
      log.error(err);
      return this.reply(425, 'No connection established');
    })
    .catch((err) => {
      log.error(err);
      this.emit('RETR', err);
      return this.reply(551, err.message);
    })
    .finally(() => {
      this.connector.end();
      this.commandSocket.resume();
    });
  },
  syntax: '{{cmd}} <path>',
  description: 'Retrieve a copy of the file'
};

[matt-forster]

@ideadesignmedia Feel free to create another PR that eclipses this one!

[andersnm]

[edit; amended my comment so as to not say anything too wrong on the internet] So it seems WinSCP expects a 150 response before STOR and RETR with TLS+passive. For some hysterical reason, WinSCP does not wait for 150 without TLS. On the other hand, FileZilla does not wait for a 150 response up front, but unlike WinSCP it breaks if you send two 150's (which my comment suggested initially, altho the PR already took care of it)