Simplified RequestGroupMan::fillRequestGroupFromReserver

src/Dependency.h

@@ -43,7 +43,7 @@ class Dependency {
 public:
   virtual ~Dependency() {}
 
-  virtual bool resolve() = 0;
+  virtual bool resolve() = 0; // throw()
 };
 
 } // namespace aria2

src/RequestGroupMan.cc

@@ -208,7 +208,11 @@ namespace {
 void notifyDownloadEvent
 (const std::string& event, const SharedHandle<RequestGroup>& group)
 {
-  SingletonHolder<Notifier>::instance()->notifyDownloadEvent(event, group);
+  // Check NULL to make unit test easier.
+  Notifier* notifier = SingletonHolder<Notifier>::instance();
+  if(notifier) {
+    notifier->notifyDownloadEvent(event, group);
+  }
 }
 
 } // namespace
@@ -486,6 +490,7 @@ void RequestGroupMan::fillRequestGroupFromReserver(DownloadEngine* e)
   while(count < num && (uriListParser_ || resitr != reservedGroups_.end())) {
     if(uriListParser_ && resitr == reservedGroups_.end()) {
       std::vector<SharedHandle<RequestGroup> > groups;
+      // May throw exception
       bool ok = createRequestGroupFromUriListParser(groups, option_,
                                                     uriListParser_.get());
       if(ok) {
@@ -501,28 +506,21 @@ void RequestGroupMan::fillRequestGroupFromReserver(DownloadEngine* e)
     }
     SharedHandle<RequestGroup> groupToAdd = (*resitr).second;
     std::vector<Command*> commands;
-    try {
-      if((rpc_ && groupToAdd->isPauseRequested()) ||
-         !groupToAdd->isDependencyResolved()) {
-        ++resitr;
-        continue;
-      }
+    if((rpc_ && groupToAdd->isPauseRequested()) ||
+       !groupToAdd->isDependencyResolved()) {
       ++resitr;
-      reservedGroups_.erase(groupToAdd->getGID());
-      // Drop pieceStorage here because paused download holds its
-      // reference.
-      groupToAdd->dropPieceStorage();
-      configureRequestGroup(groupToAdd);
-      groupToAdd->setRequestGroupMan(this);
+      continue;
+    }
+    ++resitr;
+    reservedGroups_.erase(groupToAdd->getGID());
+    // Drop pieceStorage here because paused download holds its
+    // reference.
+    groupToAdd->dropPieceStorage();
+    configureRequestGroup(groupToAdd);
+    groupToAdd->setRequestGroupMan(this);
+    try {
       createInitialCommand(groupToAdd, commands, e);
-      if(commands.empty()) {
-        requestQueueCheck();
-      }
-      groupToAdd->setState(RequestGroup::STATE_ACTIVE);
-      requestGroups_.push_back(groupToAdd->getGID(), groupToAdd);
       ++count;
-      e->addCommand(commands);
-      commands.clear();
     } catch(RecoverableException& ex) {
       A2_LOG_ERROR_EX(EX_EXCEPTION_CAUGHT, ex);
       A2_LOG_DEBUG("Deleting temporal commands.");
@@ -530,12 +528,17 @@ void RequestGroupMan::fillRequestGroupFromReserver(DownloadEngine* e)
       commands.clear();
       A2_LOG_DEBUG("Commands deleted");
       groupToAdd->setLastErrorCode(ex.getErrorCode());
-      // We add groupToAdd to e in order to it is processed in
+      // We add groupToAdd to e later in order to it is processed in
       // removeStoppedGroup().
-      groupToAdd->setState(RequestGroup::STATE_ACTIVE);
-      requestGroups_.push_back(groupToAdd->getGID(), groupToAdd);
-      requestQueueCheck();
     }
+    if(commands.empty()) {
+      requestQueueCheck();
+    } else {
+      e->addCommand(commands);
+    }
+    groupToAdd->setState(RequestGroup::STATE_ACTIVE);
+    requestGroups_.push_back(groupToAdd->getGID(), groupToAdd);
+
     util::executeHookByOptName(groupToAdd, e->getOption(),
                                PREF_ON_DOWNLOAD_START);
     notifyDownloadEvent(Notifier::ON_DOWNLOAD_START, groupToAdd);

test/RequestGroupManTest.cc

@@ -4,6 +4,7 @@
 
 #include <cppunit/extensions/HelperMacros.h>
 
+#include "TestUtil.h"
 #include "prefs.h"
 #include "DownloadContext.h"
 #include "RequestGroup.h"
@@ -16,6 +17,8 @@
 #include "array_fun.h"
 #include "RecoverableException.h"
 #include "util.h"
+#include "DownloadEngine.h"
+#include "SelectEventPoll.h"
 
 namespace aria2 {
 
@@ -27,13 +30,27 @@ class RequestGroupManTest : public CppUnit::TestFixture {
   CPPUNIT_TEST(testLoadServerStat);
   CPPUNIT_TEST(testSaveServerStat);
   CPPUNIT_TEST(testChangeReservedGroupPosition);
+  CPPUNIT_TEST(testFillRequestGroupFromReserver);
   CPPUNIT_TEST_SUITE_END();
 private:
+  SharedHandle<DownloadEngine> e_;
   SharedHandle<Option> option_;
+  SharedHandle<RequestGroupMan> rgman_;
 public:
   void setUp()
   {
     option_.reset(new Option());
+    option_->put(PREF_PIECE_LENGTH, "1048576");
+    // To enable paused RequestGroup
+    option_->put(PREF_ENABLE_RPC, A2_V_TRUE);
+    File(option_->get(PREF_DIR)).mkdirs();
+    e_.reset
+      (new DownloadEngine(SharedHandle<EventPoll>(new SelectEventPoll())));
+    e_->setOption(option_.get());
+    rgman_ = SharedHandle<RequestGroupMan>
+      (new RequestGroupMan(std::vector<SharedHandle<RequestGroup> >(),
+                           3, option_.get()));
+    e_->setRequestGroupMan(rgman_);
   }
 
   void testIsSameFileBeingDownloaded();
@@ -41,6 +58,7 @@ public:
   void testLoadServerStat();
   void testSaveServerStat();
   void testChangeReservedGroupPosition();
+  void testFillRequestGroupFromReserver();
 };
 
 
@@ -190,4 +208,25 @@ void RequestGroupManTest::testChangeReservedGroupPosition()
   }
 }
 
+void RequestGroupManTest::testFillRequestGroupFromReserver()
+{
+  SharedHandle<RequestGroup> rgs[] = {
+    createRequestGroup(0, 0, "foo1", "http://host/foo1", util::copy(option_)),
+    createRequestGroup(0, 0, "foo2", "http://host/foo2", util::copy(option_)),
+    createRequestGroup(0, 0, "foo3", "http://host/foo3", util::copy(option_)),
+    // Intentionally same path/URI for first RequestGroup and set
+    // length explicitly to do duplicate filename check.
+    createRequestGroup(0, 10, "foo1", "http://host/foo1", util::copy(option_)),
+    createRequestGroup(0, 0, "foo4", "http://host/foo4", util::copy(option_)),
+    createRequestGroup(0, 0, "foo5", "http://host/foo5", util::copy(option_))
+  };
+  rgs[1]->setPauseRequested(true);
+  for(SharedHandle<RequestGroup>* i = vbegin(rgs); i != vend(rgs); ++i) {
+    rgman_->addReservedGroup(*i);
+  }
+  rgman_->fillRequestGroupFromReserver(e_.get());
+
+  CPPUNIT_ASSERT_EQUAL((size_t)2, rgman_->getReservedGroups().size());
+}
+
 } // namespace aria2

test/TestUtil.cc

@@ -17,6 +17,9 @@
 #include "util.h"
 #include "RequestGroupMan.h"
 #include "RequestGroup.h"
+#include "DownloadContext.h"
+#include "Option.h"
+#include "FileEntry.h"
 #ifdef ENABLE_MESSAGE_DIGEST
 # include "message_digest_helper.h"
 #endif // ENABLE_MESSAGE_DIGEST
@@ -124,4 +127,21 @@ SharedHandle<RequestGroup> getReservedGroup
   return (*i).second;
 }
 
+SharedHandle<RequestGroup> createRequestGroup(int32_t pieceLength,
+                                              int64_t totalLength,
+                                              const std::string& path,
+                                              const std::string& uri,
+                                              const SharedHandle<Option>& opt)
+{
+  SharedHandle<DownloadContext> dctx(new DownloadContext(pieceLength,
+                                                         totalLength,
+                                                         path));
+  std::vector<std::string> uris;
+  uris.push_back(uri);
+  dctx->getFirstFileEntry()->addUris(uris.begin(), uris.end());
+  SharedHandle<RequestGroup> group(new RequestGroup(GroupId::create(), opt));
+  group->setDownloadContext(dctx);
+  return group;
+}
+
 } // namespace aria2

test/TestUtil.h

@@ -12,6 +12,7 @@ namespace aria2 {
 class MessageDigest;
 class RequestGroupMan;
 class RequestGroup;
+class Option;
 
 void createFile(const std::string& filename, size_t length);
 
@@ -64,4 +65,9 @@ SharedHandle<RequestGroup> findReservedGroup
 SharedHandle<RequestGroup> getReservedGroup
 (const SharedHandle<RequestGroupMan>& rgman, size_t index);
 
+SharedHandle<RequestGroup> createRequestGroup(int32_t pieceLength,
+                                              int64_t totalLength,
+                                              const std::string& path,
+                                              const std::string& uri,
+                                              const SharedHandle<Option>& opt);
 } // namespace aria2