Bug 45071 - Sendfile on APR connector can truncate downloads.
Summary: Sendfile on APR connector can truncate downloads.
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Connectors (show other bugs)
Version: 6.0.16
Hardware: Other Linux
: P2 normal (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-23 13:34 UTC by Alex Barclay
Modified: 2008-05-29 11:09 UTC (History)
0 users



Attachments
Patch to alter APR sendfile thread timeout behavior (2.91 KB, patch)
2008-05-23 13:34 UTC, Alex Barclay
Details | Diff
A better version of the patch (2.04 KB, patch)
2008-05-23 13:39 UTC, Alex Barclay
Details | Diff
Make sure that the socket_ttl array is moved down as the socket_set array is (444 bytes, patch)
2008-05-29 10:44 UTC, Alex Barclay
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Barclay 2008-05-23 13:34:29 UTC
Created attachment 21992 [details]
Patch to alter APR sendfile thread timeout behavior

The APR connector currently sets a TTL on the socket in its poller loop. Unfortunately this TTL is based on soTimeout and refers to the entire download.

This manifests itself during large file downloads (our app is around 5-10MB). The Poll.maintain(...) fires and the socket gets closed mid-transfer.

The attached patch changes this behavior such that the TTL is reset on socket activity. This now means that inactivity of 20 secs (the default from server.xml (connectionTimeout) applies to each write. So long as the client is taking data, albeit slowly, the transfer will be allowed to continue.
Comment 1 Alex Barclay 2008-05-23 13:39:14 UTC
Created attachment 21993 [details]
A better version of the patch

The first patch I attached contained this fix and another one. This patch is now correct.
Comment 2 Remy Maucherat 2008-05-25 08:24:12 UTC
This should be fixed in the native code.
Comment 3 Remy Maucherat 2008-05-25 09:33:36 UTC
(In reply to comment #1)
> Created an attachment (id=21993) [details]
> A better version of the patch
> 
> The first patch I attached contained this fix and another one. This patch is
> now correct.

I reviewed this patch, and it should work. It implies that maintain() cannot work with poll(false), while using maintain is more or less mandatory. If not fixing the native code, poll(false) needs to be removed.
Comment 4 Remy Maucherat 2008-05-26 08:58:23 UTC
Fixed in tomcat-native: http://svn.apache.org/viewvc?rev=660175&view=rev
Comment 5 Alex Barclay 2008-05-29 10:41:49 UTC
We didn't quite fix this, instead we uncovered an underlying issue.

In do_remove the list of descriptors is shuffled down as a descriptor is removed. Unfortunately the socket_ttl array was forgotten. This manifests itself as a truncated downloads on a seemingly random basis. What's actually happening is as a descriptor is removed the next higher descriptor inherits its TTL which could be close to expiry.

It would probably have been better to hold both the ttl and socket descriptor in a structure in a single array but this fix is only one line.

Comment 6 Alex Barclay 2008-05-29 10:44:17 UTC
Created attachment 22034 [details]
Make sure that the socket_ttl array is moved down as the socket_set array is

I've been testing this for a couple of days and haven't seen any more sendfile truncation. Hopefully this is the last part.
Comment 7 Mladen Turk 2008-05-29 11:09:22 UTC
Commited. Thanks!