FTP client reports successful upload in spite of an error on server
Opened this issue · 10 comments
Consider following sequence of commands for uploading a file to FTP server:
<< pasv
>> 227 Entering Passive Mode (127,0,0,1,139,38)
<< stor 5433a350-d355-11e6-9e86-d1bd614d1c5f.tar.gz
>> 150 Ok to send data
...
Client opens data connection and starts sending data. Now two things can happen:
If file is successfully saved on the server then server will close the data connection and reply with:
>> 226 Closing data connection
If there is any kind of error then server closes data connection and replies with:
>> 426 Connection closed; transfer aborted
Out FTP client ignores anything what happens after closing the data connection and because it does not explicitly check for final 226 status code, it does not notice that upload of a file has failed.
Taking into account broader picture it seems to be a general problem of jsftp implementation (directory listing and file retrieval suffer from the same problem probably) and it stems from the architectural flaw of jsftp implementation which expects FTP protocol to be simple sequence of request - response messages. From RFC 959 section 5.4:
Certain commands require a second reply for which the user should
also wait. These replies may, for example, report on the progress
or completion of file transfer or the closing of the data
connection. They are secondary replies to file transfer commands.
I came up with a patch for STOR command which fixes the problem, however the right fix would be to reimplement the core concept of queue of commands and their callbacks to something more flexible, what is capable of handling multiple responses for a single command in elegant way.
Ahh this is exactly the same problem I am running into with large file downloads #227 (I wish I had seen this earlier :P ) - Did you find a good solution?
Not a good solution but a temporary fix just for FTP uploads. You can extract the top most commit in my branch based on master and use it to patch your jsftp sources (as I do):
https://github.com/jkryl/jsftp/tree/failed-stor
let me know if it did not work. I might have done a mistake when decoupling the fix from other temporary fixes that I'm using with jsftp.
I appears from my experimentation and reading through the source that https://github.com/mscdex/node-ftp handles get,put, and list correctly
I got bitten by this problem again. This time with list command. Client does not wait for final answer from list and continues with another command before the previous one finishes and then it is off by one (accepting answer for previous command while it thinks it is for the current one). The client is then unusable.
Indeed it looks that node-ftp is handling this problem in more responsible way. However the fact that latest commit happened more than a year ago raises concern for using it in a project :)
I invested some time into this, trying to come up with a long term solution, taking into account everything what needs to be synchronised (passive connection close, reply from server, fs read/write stream close and errors). And here is the fix: https://github.com/jkryl/jsftp/tree/data-conn-races . It would be great if you could test it and let me know if it works for you before I submit a pull request.
Hey, we had the same concern about node-ftp being sort of dead. But at this point we're going to give it a whirl and fork it if we need to. We just finished our port over node-ftp
I'll take a look through that code when I get a chance.
The patch works fine for me, although I'm using it for a project where only list and put FTP commands are used. That said I think that get command works fine as well, because all BDD tests are passing. I agree I should probably submit a pull request, although I have the same concern about future of jsftp project...
has this been merged into master as I feel that I am getting a similar error today
@simonh1000 : no, it has not been merged. I lost interest in this project so I'm not sure if the patch which I provided in the past still applies cleanly on the top of the master branch. Someone either needs to take the ownership of the fix and try to convince the maintainer to merge it or keep it as a patch for private use (which is what I was doing for a year or so).