mscdex / ssh2

SSH2 client and server modules written in pure JavaScript for node.js
MIT License
5.5k stars 664 forks source link

SFTP Missing finish event #1184

Open theophilusx opened 2 years ago

theophilusx commented 2 years ago

It looks like SFTP write streams created with sftp.createWriteStream() may not be emitting a finish event. For example, with the following code

        const ws = sftp.createWriteStream(`${config.sftpUrl}/stream-t3.txt`);
        const rs = fs.createReadStream(`${config.localUrl}/test-file1.txt`);
        ws.on('error', (err) => {
          reject(err);
        });
        ws.on('finish', () => {
          console.log('ws finish event fired');
          resolve('Data streamed to remote file');
        });
        ws.on('close', () => {
          console.log('ws close event fired');
        })
        rs.pipe(ws);

only a 'close' event is triggered. If we go the reverse direction, with a fs.createWriteStream() and an sftp.creatgeReadStream(), we get first a finish event and then a close event.

This is problematic if we want to use a write stream with autoClose: false as it means no event is emitted and we cannot tell when (for example) a pipe operation ahs completed (unless we watch for events on the reader of course, but that could cause issues with knowing when all buffered data has been flushed to the write stream etc).

This was observed using ssh2 v1.10.0 on Linux running Node 18.1.0.

theophilusx commented 2 years ago

@mscdex Are you able to confirm this is an issue or am I missing something or need to provide more details?

theophilusx commented 2 years ago

@mscdex I would like to push out a new version of ssh2-sftp-client since you have released 1.11.0, but this question regarding no finish signal when closing a write stream has impact on some of the functionality in my module. Can you provide some feedback on your position with respect to this i.e. yes, it needs to be fixed, no, cannot reproduce, maybe, not sure best action if any, more information needed etc?

mscdex commented 2 years ago

I haven't had time to look at it. Node's implementation seems to change fairly often so it's difficult keeping up with it.

theophilusx commented 2 years ago

OK, thanks for the feedback.

Note that creating write streams using node streams and fs modules seem to behave correctly i.e. a finish event is raised before the close event. Only the ssh2 sftp write stream seems not to raise the finish event before the close event.

I have made some changes to my module so that things should work correctly. The one area of concern is that I've now added the ability for users to get a write stream using sftp.createWriteStream. Should they set autoClose to false on one of these streams, they will likely run into issues as they won't get any signal when for exxample, a pipe() finishes (the 'close' event is not raised if autoClose is false). Means they will have to rely on the finish signal from the reader, but that doesn't take account for possible data buffering etc (i.e. reader might be finished, but writer may not).

anzerr commented 1 year ago

I've had the same problem it appeared when I updated from node version v16.5.0 to v18.12.1

We use it like this. Both the events "close" and "finish" don't fire.

const rstream = this.client.createReadStream(oldPath, {flags: 'r', autoClose: true});
const wstream = this.client.createWriteStream(newPath, {flags: 'w', autoClose: true});
rstream.pipe(wstream);
wstream.on('end', () => {
    console.log('end was emited');
});
wstream.on('finish', () => {
    console.log('finish was emited');
});

It does seem like a change in node and now that v18 is the LTS it'll probably affect more users.

cormacbeagan commented 1 year ago

Definately seeing this issue as soon as I updated to node 18.x - can use the on 'close' handler but neither the 'end' or 'finish' handlers are being called. @anzerr that is 'end' and 'finish' are you sure you are having issues with the 'close' handler? I am just paranoid that 'close' is not going to be reliable 😅

SudhirkumarRamani commented 11 months ago

Hello @anzerr and @cormacbeagan,

I am facing same issue while upgrading my code from NodeJS 16.x to NodeJS 18.x. Could you please suggest how did you fixed or applied workaround on this issue?

Your help will be much appreciated.

ed-whittle commented 8 months ago

@SudhirkumarRamani We are reading from a file in the local storage of the container and writing to an SFTP Server. Initially we were using a Node.JS 14 AzDo pipeline, and after changing to Node 18 the finish event was no longer called. The close event now resolves the promise of the function

const readStream = fs.createReadStream(`./source_file/${fileName}`);
const writeStream = sftp.createWriteStream(remoteFileName);

readStream.pipe(writeStream);
writeStream.on('error', (err) => {
  console.error(err);
  return reject(err);
});
writeStream.on('close', () => {
  console.log('All writes complete');
  return resolve(remoteFileName.split('/')[3]);
});
// Previous Implementation pre version 18, no longer working
writeStream.on('finish', () => {
  console.log('All writes complete');
  return resolve(remoteFileName.split('/')[3]);
});
TsvetoslavVasev commented 7 months ago

Hey, any update on this, we face the exact same issue after migrating from Node 16 to Node 18, the problem is present on Node LTS as well

FranckFreiburger commented 7 months ago

i have this problem in Node 20 as well