127291 - ftp STOR command is sent after data is sent

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Kathleen :Brade]

Breaking issue off from bug 121314.
The ftp STOR command is still sent after the data has been sent.

I see this sequence (see attachments in bug 121314 too):
  TYPE I sent on stream 5
  PASV sent on stream 5
  Data sent on stream 6
  STOR sent on stream 5

Comment 1

[Bradley Baetz (:bbaetz)]

The problem is that sending a binary file as ascii is ok, but the other way
arround will fail. I could add an attribute on nsIFTPChannel, but you'd ahve to
avoid sending it for pictures. Relying on the os mimetype mappings for this is
probably not a good idea, I think. bz?

Comment 2

[Boris Zbarsky [:bzbarsky]]

So we need to be able to tell whether a file is "binary" or "ascii" for purposes
of ftp transfer?  MIME types can't possibly tell you that... consider a UTF-16
or UCS-2 encoded text/html file?

Comment 3

[Bradley Baetz (:bbaetz)]

Oops, ignore that - wrong bug.

Anyway, this shouldn't be a problem because the server should buffer. The patch
I had caused failures, so I'll have to look into this a bit more.

Comment 4

[Kathleen :Brade]

nsIChannel contains contentType; why can't you use that?

Comment 5

[Kathleen :Brade]

I will take this bug since I have a fix for this.

When I have been trying to send large images to a certain ftp server (bsd unix?)
I am seeing a hang during publish and STOR command doesn't get sent.  I am
guessing that the server is refusing to accept more data because it doesn't know
where to put the data (due to missing STOR).  The STOR doesn't get sent because
the ftp control transport is waiting for the data pipe to be "connected"

Reworking the ftp code to always initiate a read on the data pipe transport when
it is created causes the data pipe's tcp connection to be opened.  That allows
the control transport to send the pending STOR command and the response to the
STOR command causes us to send the data.

The first portion of the patch is from earlier work by bbaetz where we don't
start sending the data unless we get a 1xx response for the STOR command.  This
ensures that the STOR command is sent before we start sending data to the server.

Comment 6

[Kathleen :Brade]

Attachment: always send STOR before data; always read to open data connection

Comment 7

[Kathleen :Brade]

Darin--please sr=

Comment 8

[Bradley Baetz (:bbaetz)]

Ah, ok.

r=bbaetz too, FWIW.

Comment 9

[Darin Fisher]

Comment on attachment 76197 [details] [diff] [review]
always send STOR before data; always read to open data connection

>Index: mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp

>+    NS_ASSERTION(mWriteStream, "we're trying to upload without any data");
>+    if (!mWriteStream)
>+        return NS_ERROR_FAILURE;

nit: why test mWriteStream twice?  sure, debug only, but it's not very clean...

      if (!mWriteStream) {
	  NS_ERROR(...);
	  return NS_ERROR_FAILURE;
      }


>+    if (mResponseCode/100 == 1) {
>+        PRUint32 len;
>+        mWriteStream->Available(&len);

how is data entering the pipe?	do you know that more data won't enter after
this
call to Available?  and if it does, could that be a problem?

sr=darin

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 76197 [details] [diff] [review]
always send STOR before data; always read to open data connection

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 11

[Kathleen :Brade]

>how is data entering the pipe?	do you know that more data won't enter after
>this call to Available?  and if it does, could that be a problem?

The data for uploading is already there; the writeStream was initialized much
earlier and in the case of Composer the data is all there and ready to go (http
publishing needs the size for the headers).  I don't think that this is a
problem or at least not a new problem since this patch just moved existing code
around (from S_stor() to R_stor()).

patch checked in.

Comment 12

[Darin Fisher]

brade: ok, i didn't realize we were talking about the post data stream. sounds good.

Comment 13

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old