PUT method, files, and chunked encoding

[Michael Smith <ms...@xn.com.au>]

Hi all,

I've run into a problem with the PUT method. Since the webdav servlet
currently doesn't deal correctly with the chunked encoding (ok, so this
is the main problem, but it's easier to fix the immediate issues
elsewhere), things go bad if one is using the client library, and it
uses a chunked encoding. 

If you try to PUT a file (using the relevent constructor for PutMethod),
the content-length doesn't get sent. As a result, the client library
later decides to send the data chunked (if talking HTTP/1.1). So, the
file ends up corrupted on the server. Not good.

Tracking this further, PutMethod used to set the content length so that
this didn't happen. However, a commit in early december commented this
out with the (not so) enlightening commit meesage "Don't set content
length while sending a file (temp fix)." 

What was this a temp fix for? Is it still needed? Looking at the code,
it seems like precisely the right thing to do. 

Michael

Re: PUT method, files, and chunked encoding

[Remy Maucherat <re...@apache.org>]

> > Feel free to suggest changes. Using chunking is fine IMO (it is a
> > requirement for HTTP compliance, and is well supported by almost every
> > server out there).
>
> Yes, using chunking here is (in general) the right thing to do - but for
> the specific case I was looking at (streaming a file), the
> content-length can be known in advance, in which case it's better to not
> use chunking, I think. Either way is acceptable if it works, though.

I agree that setting the CL is the way to go here. That was what I was doing
before, but then I noticed it was buggy (chunking + CL = BAD !), so I
removed the setContentLength, which was the easiest way to fix the bug.
Unfortunately, I only tested with small files (so I didn't see the bug you
reported on the server side), and I was also doing other ops before making
the PUT (which allows to bypass the auth challenges / response retries).

> Thanks for the rest of your reply - it's made things clearer to me. For
> the moment, I'll just use the non-streaming methods, which work
> (inefficient, but this won't be a long term solution, so it's not too
> big a deal). The webdav command line client has the same problem (PUT
> doesn't work, as a result).

I'll update the request algorithm soon and try to resolve that (hopefully
without using buffering).

Remy

Re: PUT method, files, and chunked encoding

[Michael Smith <ms...@xn.com.au>]

> 
> Feel free to suggest changes. Using chunking is fine IMO (it is a
> requirement for HTTP compliance, and is well supported by almost every
> server out there).

Yes, using chunking here is (in general) the right thing to do - but for
the specific case I was looking at (streaming a file), the
content-length can be known in advance, in which case it's better to not
use chunking, I think. Either way is acceptable if it works, though.

Thanks for the rest of your reply - it's made things clearer to me. For
the moment, I'll just use the non-streaming methods, which work
(inefficient, but this won't be a long term solution, so it's not too
big a deal). The webdav command line client has the same problem (PUT
doesn't work, as a result).

Michael

Re: PUT method, files, and chunked encoding

[Remy Maucherat <re...@apache.org>]

> Ok. I guess I misread the code there - it looked to me like it wouldn't
> do chunking if the content-length header was set. However, the problem
> remains - using chunking currently doesn't seem to work (problems on the
> server side, I think), but the client library tries to use it. Can you
> recommend a sensible workaround for the moment?

I just fixed the bug in the Tomcat 4.1 workspace. It will be rollbacked at
some point in the TC 4 codebase, but beta 2 is probably still a few weeks
away.

There is no workaround without updating the client to expose the some
capabitlities flags. I'll add that ASAP.
Slide should run fine with TC 3.2 (which advertises itself as HTTP/1.0), so
that could ba a possible workaround.

> That aside, there seem to be more critical problems with giving a file
> to the client libraries (well, with any of the methods that ultimately
> use the stream stuff, rather than a byte array). I think (but am not
> sure - I don't know the code well enough yet) that there are a couple of
> issues, but the others may all be caused by one main problem.

Feel free to suggest changes. Using chunking is fine IMO (it is a
requirement for HTTP compliance, and is well supported by almost every
server out there).

> That problem is that, as things are now, a typical exchange goes like
> this (This is what I think I'm seeing, anyway - I might be way off with
> some of my speculation here) (with authentication required - things work
> fine otherwise):
>
>   Attempt to PUT file:
>     send headers
>     send body
>   Read response
>     socket has been closed already by server (because it isn't
> authorized). Exception thrown and dealt with silently.
>   Retry above.
>     send headers
>     try to send body. body has already been read, file is at EOF, so
> nothing is sent. However, since it's still unauthorized, things don't
> break yet.
>   Read response: 401 Unauthorized
>   Attempt to PUT file for the 3rd time, this time with authentication
> headers
>     send headers.
>     send body (again, file is at EOF, nothing is sent).
>   Read response
>     Server hasn't seen any body yet, so is waiting. client hangs on a
> blocking socket read.
>
> At this point we can't do anything much (unless the socket times out -
> I'm not sure how java deals with that, but it's only a problem because
> of earlier breakage).
>
> So, we see the following problems:
> Firstly, client attempts to send the body of the request even if the
> server has responded beforehand (e.g. immediately after the headers end,
> the server sends 401 Unauthorized - it doesn't wait for the body). This
> seems inefficient, and results in an exception being silently swallowed
> afterwards.

Yes, that should be enhanced, but I really don't see how. Most requests will
only produce a response once the complete request has been sent - including
the body (except if there's an auth challenge). I'll try to find something.
That feature will be at the top of my list for the next milestone.

> Secondly, and most crucially, the data at this point has been 'used up'
> - the InputStream has reached the end, and java doesn't provide us with
> a method to rewind to the start, or otherwise seek (fair enough - it's
> not always possible). As a result, subsequent attempts (and as the code
> is structured currently, subsequent attempts are ALWAYS needed if
> authentication is being used) fail horribly.
>
> Thirdly, we end up hanging because of the above.

Indeed, that sequence would break it.

> So, a couple of questions: why don't we just authenticate the first time
> around (a look at the code makes me suspect HTTP requires this

Yes, you have to wait to wait to recieve an auth challenge to send the
credentials.

> , but I
> don't know the details of HTTP authentication)? Next, what's a sensible
> way to solve this problem?

If we wait to read the response headers before sending the body, it should
be ok. I think buffering should be used then, either memory or filesystem
(user's option), unless I can figure out a better solution.

> It only seems to be a problem with PUT requests (that I've looked at so
> far - I suppose other things like PROPPATCH could have similar problems,
> but that's usually from a memory buffer, not an InputStream), so
> completely restructuring the core code in WebdavClient.executeMethod()
> seems like the wrong way to be going about things - but I don't see how
> else to do it.

No that's ok. The library is meant to be extensible, so we shouldn't assume
that most requests won't use streaming.

> Alternatively, should I (for now, at least) just give up on using files
> (and thus InputStreams) and just pass the client library a (potentially
> very large) memory buffer?

Indeed, the buffer is potentially very large, so that's why I a buffering to
disk option should be present as well (if buffering turns out to be
necessary).

Remy

Re: PUT method, files, and chunked encoding

[Michael Smith <ms...@xn.com.au>]

> > What was this a temp fix for? Is it still needed? Looking at the code,
> > it seems like precisely the right thing to do.
> 
> No, it should work anyway. It's a bug in Tomcat 4, so I won't fix anything
> is Slide.
> 
> I wrote in the commit message that it's temporary, because it's a fix
> dealing with how the chunking flag value is determined (it should be
> smarter). If you try to set the content-length here, the request will have a
> content-length header *and* still be chunked, which of course is invalid.
> Not using chunking would enhance performance a little bit (and also fix
> compatibility with broken HTTP/1.1 servers).
> 
> Remy

Ok. I guess I misread the code there - it looked to me like it wouldn't
do chunking if the content-length header was set. However, the problem
remains - using chunking currently doesn't seem to work (problems on the
server side, I think), but the client library tries to use it. Can you
recommend a sensible workaround for the moment?

That aside, there seem to be more critical problems with giving a file
to the client libraries (well, with any of the methods that ultimately
use the stream stuff, rather than a byte array). I think (but am not
sure - I don't know the code well enough yet) that there are a couple of
issues, but the others may all be caused by one main problem.

That problem is that, as things are now, a typical exchange goes like
this (This is what I think I'm seeing, anyway - I might be way off with
some of my speculation here) (with authentication required - things work
fine otherwise):

  Attempt to PUT file:
    send headers
    send body
  Read response
    socket has been closed already by server (because it isn't
authorized). Exception thrown and dealt with silently.
  Retry above.
    send headers
    try to send body. body has already been read, file is at EOF, so
nothing is sent. However, since it's still unauthorized, things don't
break yet.
  Read response: 401 Unauthorized
  Attempt to PUT file for the 3rd time, this time with authentication
headers
    send headers.
    send body (again, file is at EOF, nothing is sent).
  Read response
    Server hasn't seen any body yet, so is waiting. client hangs on a
blocking socket read.

At this point we can't do anything much (unless the socket times out -
I'm not sure how java deals with that, but it's only a problem because
of earlier breakage).

So, we see the following problems:
Firstly, client attempts to send the body of the request even if the
server has responded beforehand (e.g. immediately after the headers end,
the server sends 401 Unauthorized - it doesn't wait for the body). This
seems inefficient, and results in an exception being silently swallowed
afterwards.

Secondly, and most crucially, the data at this point has been 'used up'
- the InputStream has reached the end, and java doesn't provide us with
a method to rewind to the start, or otherwise seek (fair enough - it's
not always possible). As a result, subsequent attempts (and as the code
is structured currently, subsequent attempts are ALWAYS needed if
authentication is being used) fail horribly. 

Thirdly, we end up hanging because of the above.

So, a couple of questions: why don't we just authenticate the first time
around (a look at the code makes me suspect HTTP requires this, but I
don't know the details of HTTP authentication)? Next, what's a sensible
way to solve this problem? 

It only seems to be a problem with PUT requests (that I've looked at so
far - I suppose other things like PROPPATCH could have similar problems,
but that's usually from a memory buffer, not an InputStream), so
completely restructuring the core code in WebdavClient.executeMethod()
seems like the wrong way to be going about things - but I don't see how
else to do it. 

Alternatively, should I (for now, at least) just give up on using files
(and thus InputStreams) and just pass the client library a (potentially
very large) memory buffer? 

Michael

Re: PUT method, files, and chunked encoding

[Remy Maucherat <re...@apache.org>]

> Hi all,
>
> I've run into a problem with the PUT method. Since the webdav servlet
> currently doesn't deal correctly with the chunked encoding (ok, so this
> is the main problem, but it's easier to fix the immediate issues
> elsewhere), things go bad if one is using the client library, and it
> uses a chunked encoding.
>
> If you try to PUT a file (using the relevent constructor for PutMethod),
> the content-length doesn't get sent. As a result, the client library
> later decides to send the data chunked (if talking HTTP/1.1). So, the
> file ends up corrupted on the server. Not good.
>
> Tracking this further, PutMethod used to set the content length so that
> this didn't happen. However, a commit in early december commented this
> out with the (not so) enlightening commit meesage "Don't set content
> length while sending a file (temp fix)."
>
> What was this a temp fix for? Is it still needed? Looking at the code,
> it seems like precisely the right thing to do.

No, it should work anyway. It's a bug in Tomcat 4, so I won't fix anything
is Slide.

I wrote in the commit message that it's temporary, because it's a fix
dealing with how the chunking flag value is determined (it should be
smarter). If you try to set the content-length here, the request will have a
content-length header *and* still be chunked, which of course is invalid.
Not using chunking would enhance performance a little bit (and also fix
compatibility with broken HTTP/1.1 servers).

Remy