mscdex / node-ftp

An FTP client module for node.js
MIT License
1.13k stars 243 forks source link

Why ftp.list runs my callback twice?! #44

Closed outluch closed 11 years ago

outluch commented 11 years ago
    source.once('end', ondone);
    source.once('close', ondone);

Theese lines seems running it twice. Please do smth. Thanks.

mscdex commented 11 years ago

Can you provide the code you are using? Is this a public FTP server? I've personally never run into this issue.

outluch commented 11 years ago

unfortunately i deleted it. Went other way in my program. And FTP is private. But i can explayn: I just tryed to .list some directory just like in example in README.MD.

hitsthings commented 11 years ago

I'm hitting this too. The FTP server isn't public, but here's the stack traces from the two callbacks so you can compare the codepaths:

Error
    at C:\Code\live-ftp-upload\index.js:55:25
    at final (C:\Code\live-ftp-upload\node_modules\ftp\lib\ftp.js:421:11)
    at Object.FTP.list [as cb] (C:\Code\live-ftp-upload\node_modules\ftp\lib\ftp
.js:456:11)
    at Socket.ondata (C:\Code\live-ftp-upload\node_modules\ftp\lib\ftp.js:258:22
)
    at Socket.EventEmitter.emit (events.js:96:17)
    at TCP.onread (net.js:391:31)
Error
    at C:\Code\live-ftp-upload\index.js:55:25
    at final (C:\Code\live-ftp-upload\node_modules\ftp\lib\ftp.js:421:11)
    at Socket.ondone (C:\Code\live-ftp-upload\node_modules\ftp\lib\ftp.js:401:7)

    at Socket.g (events.js:192:14)
    at Socket.EventEmitter.emit (events.js:126:20)
    at Socket._destroy.destroyed (net.js:357:10)
    at process.startup.processNextTick.process._tickCallback (node.js:244:9)

Possibly relevant: I'm on Windows.

I think ondone just needs an if(!done) {...} wrapper

ghost commented 11 years ago

I have the same problem... "list" callback is called twice, or sometimes never... I've traced why no-data occurs: my ftp server returns: 150 Accepted data connection 226-Options: -a -l 226 11 matches total

in one chunk, and in the callback in self._send in sendList() you got: code=226 text=150 Accepted data connection,

insted of 2 callbacks: first with 150 code, second with 226 code. When you get only code=226 first, you close socket (sock.destroy) so no data arrives.

Didn't trace why duplicate callback occurs, maybe it is the same source of the problem...

nicolashery commented 11 years ago

Running into the same issue: c.list(callback) runs the callback twice.

Like @hitsthings, I'm on Windows. Will try to test same code on a Mac later, to see if the same thing happens...

Update: I just tested on my Mac, I get the exact same issue. So it doesn't seem to be Windows related. The private FTP server I'm connecting to is from an enterprise Box.com account.

mscdex commented 11 years ago

@lukfol that particular problem should be now fixed in 13dd906

mscdex commented 11 years ago

Anyone else that is still having this issue, can you please try applying this patch on top of current master and let me know if that resolves things for you?

nicolashery commented 11 years ago

Works great after applying patch to https://github.com/mscdex/node-ftp/commit/19331c3592425ab289cf294aa056e2ae0c7c2595! :)

Thanks a lot @mscdex!

Cheers, Nicolas

mscdex commented 11 years ago

@hitsthings can you also confirm the patch works for you as well?

hitsthings commented 11 years ago

I'll check it out after work (it's 12:30pm, Sydney time). Don't have my code/credentials nearby at the moment.

ghost commented 11 years ago

Yes, problem with duplicated callbacks is fixed. But I still have problem with getting void list, so I retry and retry list call until i get not empty one :) I have ftp connection in local network, so the problem is that as ftp client I receive 150 Accepted data connection 226-Options: -a -l in one chunk- BEFORE tcp packet with directory list arrives, since it is separate tcp connection. I suggesty to check for status only for errors, and if no errors- wait for the data connection to close collecting all data.

mscdex commented 11 years ago

@lukfol So you're saying when you request a list those two lines show up first, then the directory listing is sent, and then the second 226 line shows up? Do you know what server software this is so I can test locally?

ghost commented 11 years ago

Client receives all 3 lines in one chunk:

150 Accepted data connection 226-Options: -a -l 226 11 matches total

and the callback in self._send receives: code = 226 text = 150 Accepted data connection

(the third line from data chunk is ignored by parser), then connection is closed and empty list is returned... I think those data would arrive if you don't close connection. I don't think it's the matter of server (it's backuping server in OVH provider)- rather it's becouse it's local network, width Nagle's algorithm on- which collects all 3 lines into one chunk, before it is sent. The server could wait with sending 226 line after data is received by client in passive mode, but I don't know if it is requeired by protocol.

WiadomoϾ napisana przez Brian White w dniu 8 mar 2013, o godz. 13:38:

@lukfol So you're saying when you request a list those two lines show up first, then the directory listing is sent, and then the second 226 line shows up? Do you know what server software this is so I can test locally?

— Reply to this email directly or view it on GitHub.

mscdex commented 11 years ago

@lukfol did you try the current master branch? you should not still be getting code 226 and text set to "150 Accepted data connection."

ghost commented 11 years ago

I still have the same problem... It's the matter of parsing in function ondata(chunk) {

when you get all 3 lines in one chunk.

Maybe I could create an account on my server so you could test on it (connect to the ftp)?

WiadomoϾ napisana przez Brian White w dniu 8 mar 2013, o godz. 15:34:

@lukfol did you try the current master branch? you should not still be getting code 226 and text set to "150 Accepted data connection."

— Reply to this email directly or view it on GitHub.

mscdex commented 11 years ago

@lukfol The problem is that it shouldn't be possible to get both a 150 and 226 in the same chunk, unless the server is really broken or I'm really missing something here.

I think being able to test it directly from my end would be really helpful since I cannot duplicate this locally or on other public ftp servers I have tried.

EDIT: regarding your comments from earlier, 226 basically means "data connection closed" and should only be sent by the server once 150 and the listing have been sent first.

ghost commented 11 years ago

Maybe it's not right behaviuour, but it happens :)

Why couldn't you just wait for passive connection to close?

WiadomoϾ napisana przez Brian White w dniu 8 mar 2013, o godz. 16:32:

@lukfol The problem is that it shouldn't be possible to get both a 150 and 226 in the same chunk, unless the server is really broken or I'm really missing something here.

I think being able to test it directly from my end would be really helpful since I cannot duplicate this locally or on other public ftp servers I have tried.

— Reply to this email directly or view it on GitHub.

mscdex commented 11 years ago

@lukfol That is basically what the module does. However I believe the reason why the socket may have been explicitly closed from the client side was due to a combination of a bug in the parsing (which is now fixed) and the fact that some servers will send 226 immediately with no preceding 150/125 on empty directories.

mscdex commented 11 years ago

@lukfol I didn't see your email before I posted that. It should be working now with the master branch. I had missed a small change I made in a regexp when I was doing some separate, local testing earlier.

ghost commented 11 years ago

No it works good! No duplicate callbacks and no empty list (when directory is not empty). Thanks!

WiadomoϾ napisana przez Brian White w dniu 8 mar 2013, o godz. 17:43:

@lukfol I didn't see your email before I posted that. It should be working now with the master branch. I had missed a small change I made in a regexp when I was doing some separate, local testing earlier.

— Reply to this email directly or view it on GitHub.

hitsthings commented 11 years ago

Thanks! New version definitely does the trick.