Do not leak Downloader-related objects.

The downloadFinished() method was responsible for the job clean up, but
that asynchronous method is not called when the Downloader job quits
before the call can be fired. This early termination happens when, for
example, the job finishes while still inside the start() method (e.g., a
memory hit with no async ACLs to check). It also happens if an exception
is thrown while the job is running under async call protections.

Ensure the cleanup happens regardless of the job path to finish.

This is a Measurement Factory project.

diff --git a/src/Downloader.cc b/src/Downloader.cc
index a9320b37758c13849ee42cf9f0ee9496a23c3d83..6847094eabd76d22c84d5a8bb013aa2996a4453b 100644 (file)
--- a/src/Downloader.cc
+++ b/src/Downloader.cc
@@ -73,6 +73,17 @@ Downloader::Downloader(SBuf &url, AsyncCall::Pointer &aCallback, unsigned int le
 
 Downloader::~Downloader()
 {
+    debugs(33, 6, this);
+}
+
+void
+Downloader::swanSong()
+{
+    debugs(33, 6, this);
+    if (context_) {
+        context_->finished();
+        context_ = nullptr;
+    }
 }
 
 bool
@@ -234,12 +245,6 @@ void
 Downloader::downloadFinished()
 {
     debugs(33, 7, this);
-    // We cannot delay http destruction until refcounting deletes
-    // DownloaderContext. The http object destruction will cause
-    // clientStream cleanup and will release the refcount to context_
-    // object hold by clientStream structures.
-    context_->finished();
-    context_ = nullptr;
     Must(done());
 }
 
@@ -256,12 +261,10 @@ Downloader::callBack(Http::StatusCode const statusCode)
     ScheduleCallHere(callback_);
     callback_ = nullptr;
 
-    // Calling deleteThis method here to finish Downloader
-    // may result to squid crash.
-    // This method called by handleReply method which maybe called
-    // by ClientHttpRequest::doCallouts. The doCallouts after this object
-    // deleted, may operate on non valid objects.
-    // Schedule an async call here just to force squid to delete this object.
+    // We cannot deleteThis() because we may be called synchronously from
+    // doCallouts() via handleReply() (XXX), and doCallouts() may crash if we
+    // disappear. Instead, schedule an async call now so that later, when the
+    // call firing code discovers a done() job, it deletes us.
     CallJobHere(33, 7, CbcPointer<Downloader>(this), Downloader, downloadFinished);
 }
 

diff --git a/src/Downloader.h b/src/Downloader.h
index 7c681ed3cbf57335aa84b01ccaa92d3d0fa66184..249774620fd2b81f84dfe2804e2f384c09484bff 100644 (file)
--- a/src/Downloader.h
+++ b/src/Downloader.h
@@ -47,6 +47,7 @@ public:
 
     Downloader(SBuf &url, AsyncCall::Pointer &aCallback, unsigned int level = 0);
     virtual ~Downloader();
+    virtual void swanSong();
 
     /// delays destruction to protect doCallouts()
     void downloadFinished();