2008-12-16 Tatsuhiro Tsujikawa <t-tujikawa@users.sourceforge.net>

Fixed the bug that causes corrupted downloads if HTTP pipelining
	is enabled and the server doesn't support keep-alive.
	* src/AbstractCommand.cc
	* src/DownloadCommand.cc
	* src/HttpDownloadCommand.cc
	* src/HttpDownloadCommand.h
	* src/HttpResponseCommand.cc
	* src/Request.cc
	* src/Request.h
	* test/RequestTest.cc

ChangeLog

@@ -1,3 +1,16 @@
+2008-12-16  Tatsuhiro Tsujikawa  <t-tujikawa@users.sourceforge.net>
+
+	Fixed the bug that causes corrupted downloads if HTTP pipelining
+	is enabled and the server doesn't support keep-alive.
+	* src/AbstractCommand.cc
+	* src/DownloadCommand.cc
+	* src/HttpDownloadCommand.cc
+	* src/HttpDownloadCommand.h
+	* src/HttpResponseCommand.cc
+	* src/Request.cc
+	* src/Request.h
+	* test/RequestTest.cc
+
 2008-12-14  Tatsuhiro Tsujikawa  <t-tujikawa@users.sourceforge.net>
 
 	Removed Dictionary/List/Data and its related classes.

src/AbstractCommand.cc

@@ -123,13 +123,7 @@ bool AbstractCommand::execute() {
       if(!_requestGroup->getPieceStorage().isNull()) {
 	_segments.clear();
 	_requestGroup->getSegmentMan()->getInFlightSegment(_segments, cuid);
-	size_t maxSegments;
+	while(_segments.size() < req->getMaxPipelinedRequest()) {
-	if(req->isPipeliningEnabled()) {
-	  maxSegments = e->option->getAsInt(PREF_MAX_HTTP_PIPELINING);
-	} else {
-	  maxSegments = 1;
-	}
-	while(_segments.size() < maxSegments) {
 	  SegmentHandle segment = _requestGroup->getSegmentMan()->getSegment(cuid);
 	  if(segment.isNull()) {
 	    break;

src/DownloadCommand.cc

@@ -263,7 +263,9 @@ bool DownloadCommand::prepareForNextSegment() {
 #endif // ENABLE_MESSAGE_DIGEST
     return true;
   } else {
-    if(_segments.size()) {
+    // The number of segments should be 1 in order to pass through the next
+    // segment.
+    if(_segments.size() == 1) {
       SegmentHandle tempSegment = _segments.front();
       SegmentHandle nextSegment =
 	_requestGroup->getSegmentMan()->getSegment(cuid,

src/HttpDownloadCommand.cc

@@ -43,23 +43,37 @@
 #include "Socket.h"
 #include "prefs.h"
 #include "Option.h"
+#include "HttpResponse.h"
+#include "HttpHeader.h"
+#include "Range.h"
 
 namespace aria2 {
 
-HttpDownloadCommand::HttpDownloadCommand(int cuid,
+HttpDownloadCommand::HttpDownloadCommand
+(int cuid,
  const RequestHandle& req,
  RequestGroup* requestGroup,
+ const SharedHandle<HttpResponse>& httpResponse,
  const HttpConnectionHandle& httpConnection,
  DownloadEngine* e,
  const SocketHandle& socket)
   :DownloadCommand(cuid, req, requestGroup, e, socket),
+   _httpResponse(httpResponse),
    _httpConnection(httpConnection) {}
 
 HttpDownloadCommand::~HttpDownloadCommand() {}
 
 bool HttpDownloadCommand::prepareForNextSegment() {
-  if(req->isPipeliningEnabled() && !_requestGroup->downloadFinished()) {
+  bool downloadFinished = _requestGroup->downloadFinished();
-    Command* command = new HttpRequestCommand(cuid, req, _requestGroup, _httpConnection, e, socket);
+  if(req->isPipeliningEnabled() && !downloadFinished) {
+    HttpRequestCommand* command =
+      new HttpRequestCommand(cuid, req, _requestGroup, _httpConnection, e,
+			     socket);
+    // Set proxy request here. aria2 sends the HTTP request specialized for
+    // proxy.
+    if(e->option->get(PREF_PROXY_METHOD) == V_GET) {
+      command->setProxyRequest(createProxyRequest());
+    }
     e->commands.push_back(command);
     return true;
   } else {
@@ -71,7 +85,20 @@ bool HttpDownloadCommand::prepareForNextSegment() {
 	 _requestGroup->getTotalLength()))) {
       e->poolSocket(req, isProxyDefined(), socket);
     }
-
+    // The request was sent assuming that server supported pipelining, but
+    // it turned out that server didn't support it.
+    // We detect this situation by comparing the end byte in range header
+    // of the response with the end byte of segment.
+    // If it is the same, HTTP negotiation is necessary for the next request.
+    if(!req->isPipeliningEnabled() && req->isPipeliningHint() &&
+       !_segments.empty() && !downloadFinished) {
+      const SharedHandle<Segment>& segment = _segments.front();
+      if(segment->getPosition()+segment->getLength() ==
+	 static_cast<uint64_t>(_httpResponse->getHttpHeader()->
+			       getRange()->getEndByte()+1)) {
+	return prepareForRetry(0);
+      }
+    }
     return DownloadCommand::prepareForNextSegment();
   }
 }

src/HttpDownloadCommand.h

@@ -39,10 +39,12 @@
 
 namespace aria2 {
 
+class HttpResponse;
 class HttpConnection;
 
 class HttpDownloadCommand : public DownloadCommand {
 private:
+  SharedHandle<HttpResponse> _httpResponse;
   SharedHandle<HttpConnection> _httpConnection;
 protected:
   virtual bool prepareForNextSegment();
@@ -50,6 +52,7 @@ public:
   HttpDownloadCommand(int cuid,
 		      const SharedHandle<Request>& req,
 		      RequestGroup* requestGroup,
+		      const SharedHandle<HttpResponse>& httpResponse,
 		      const SharedHandle<HttpConnection>& httpConnection,
 		      DownloadEngine* e,
 		      const SharedHandle<SocketCore>& s);

src/HttpResponseCommand.cc

@@ -100,6 +100,9 @@ bool HttpResponseCommand::executeInternal()
   // We don't care whether non-HTTP/1.1 server returns Connection: keep-alive.
   req->supportsPersistentConnection
     (httpResponse->supportsPersistentConnection());
+  if(req->isPipeliningEnabled()) {
+    req->setMaxPipelinedRequest(e->option->getAsInt(PREF_MAX_HTTP_PIPELINING));
+  }
 
   if(httpResponse->getResponseStatus() >= HttpHeader::S300) {
     if(httpResponse->getResponseStatus() == HttpHeader::S404) {
@@ -328,8 +331,8 @@ HttpDownloadCommand* HttpResponseCommand::createHttpDownloadCommand
 {
 
   HttpDownloadCommand* command =
-    new HttpDownloadCommand(cuid, req, _requestGroup, httpConnection, e,
+    new HttpDownloadCommand(cuid, req, _requestGroup,
-			    socket);
+			    httpResponse, httpConnection, e, socket);
   command->setMaxDownloadSpeedLimit
     (e->option->getAsInt(PREF_MAX_DOWNLOAD_LIMIT));
   command->setStartupIdleTime(e->option->getAsInt(PREF_STARTUP_IDLE_TIME));

src/Request.cc

@@ -69,6 +69,7 @@ Request::Request():
   _supportsPersistentConnection(true),
   _keepAliveHint(false),
   _pipeliningHint(false),
+  _maxPipelinedRequest(1),
   method(METHOD_GET)
 {}
 
@@ -115,12 +116,14 @@ static std::string urlencode(const std::string& src)
 }
 
 bool Request::setUrl(const std::string& url) {
+  _supportsPersistentConnection = true;
   this->url = url;
   return parseUrl(urlencode(removeFragment(url)));
 }
 
 bool Request::resetUrl() {
   previousUrl = referer;
+  _supportsPersistentConnection = true;
   return parseUrl(urlencode(removeFragment(url)));
 }
 
@@ -243,4 +246,19 @@ unsigned int Request::getRedirectCount() const
   return _redirectCount;
 }
 
+bool Request::isPipeliningHint() const
+{
+  return _pipeliningHint;
+}
+
+void Request::setMaxPipelinedRequest(unsigned int num)
+{
+  _maxPipelinedRequest = num;
+}
+
+unsigned int Request::getMaxPipelinedRequest() const
+{
+  return _maxPipelinedRequest;
+}
+
 } // namespace aria2

src/Request.h

@@ -72,6 +72,8 @@ private:
   bool _keepAliveHint;
   // enable pipelining if possible.
   bool _pipeliningHint;
+  // maximum number of pipelined requests
+  unsigned int _maxPipelinedRequest;
 
   std::string method;
 
@@ -144,6 +146,12 @@ public:
     _pipeliningHint = pipeliningHint;
   }
 
+  bool isPipeliningHint() const;
+
+  void setMaxPipelinedRequest(unsigned int num);
+
+  unsigned int getMaxPipelinedRequest() const;
+
   void setMethod(const std::string& method) {
     this->method = method;
   }

test/RequestTest.cc

@@ -31,9 +31,12 @@ class RequestTest:public CppUnit::TestFixture {
   CPPUNIT_TEST(testSetUrl_username);
   CPPUNIT_TEST(testSetUrl_usernamePassword);
   CPPUNIT_TEST(testSetUrl_zeroUsername);
+  CPPUNIT_TEST(testSetUrl_supportsPersistentConnection);
   CPPUNIT_TEST(testRedirectUrl);
   CPPUNIT_TEST(testRedirectUrl2);
+  CPPUNIT_TEST(testRedirectUrl_supportsPersistentConnection);
   CPPUNIT_TEST(testResetUrl);
+  CPPUNIT_TEST(testResetUrl_supportsPersistentConnection);
   CPPUNIT_TEST(testInnerLink);
   CPPUNIT_TEST(testInnerLinkInReferer);
   CPPUNIT_TEST_SUITE_END();
@@ -59,9 +62,12 @@ public:
   void testSetUrl_username();
   void testSetUrl_usernamePassword();
   void testSetUrl_zeroUsername();
+  void testSetUrl_supportsPersistentConnection();
   void testRedirectUrl();
   void testRedirectUrl2();
+  void testRedirectUrl_supportsPersistentConnection();
   void testResetUrl();
+  void testResetUrl_supportsPersistentConnection();
   void testInnerLink();
   void testInnerLinkInReferer();
 };
@@ -447,4 +453,34 @@ void RequestTest::testSetUrl_usernamePassword()
 
 }
 
+void RequestTest::testSetUrl_supportsPersistentConnection()
+{
+  Request req;
+  CPPUNIT_ASSERT(req.setUrl("http://host/file"));
+  req.supportsPersistentConnection(false);
+  CPPUNIT_ASSERT(!req.supportsPersistentConnection());
+  req.setUrl("http://host/file");
+  CPPUNIT_ASSERT(req.supportsPersistentConnection());
+}
+
+void RequestTest::testResetUrl_supportsPersistentConnection()
+{
+  Request req;
+  CPPUNIT_ASSERT(req.setUrl("http://host/file"));
+  req.supportsPersistentConnection(false);
+  CPPUNIT_ASSERT(!req.supportsPersistentConnection());
+  req.resetUrl();
+  CPPUNIT_ASSERT(req.supportsPersistentConnection());
+}
+
+void RequestTest::testRedirectUrl_supportsPersistentConnection()
+{
+  Request req;
+  CPPUNIT_ASSERT(req.setUrl("http://host/file"));
+  req.supportsPersistentConnection(false);
+  CPPUNIT_ASSERT(!req.supportsPersistentConnection());
+  req.redirectUrl("http://host/file");
+  CPPUNIT_ASSERT(req.supportsPersistentConnection());
+}
+
 } // namespace aria2