All users were logged out of Bugzilla on October 13th, 2018

ftp STOR command is sent after data is sent

RESOLVED FIXED in mozilla1.0

Status

()

RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: Brade, Assigned: Brade)

Tracking

Trunk
mozilla1.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: publish)

Attachments

(1 attachment)

(Assignee)

Description

17 years ago
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
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?
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?
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.
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 4

17 years ago
nsIChannel contains contentType; why can't you use that?
Target Milestone: mozilla1.0 → ---
(Assignee)

Comment 5

17 years ago
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.
Assignee: bbaetz → brade
Whiteboard: publish
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 6

17 years ago
Created attachment 76197 [details] [diff] [review]
always send STOR before data; always read to open data connection

Updated

17 years ago
Attachment #76197 - Flags: review+
(Assignee)

Comment 7

17 years ago
Darin--please sr=
Status: NEW → ASSIGNED
Keywords: nsbeta1
Ah, ok.

r=bbaetz too, FWIW.

Comment 9

17 years ago
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
Attachment #76197 - Flags: superreview+

Comment 10

17 years ago
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
Attachment #76197 - Flags: approval+
(Assignee)

Comment 11

17 years ago
>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.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 12

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

Updated

17 years ago
Keywords: verifyme
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.