From: phk <phk@d4fa192b-c00b-0410-8231-f00ffab90ce4>
Date: Wed, 14 Jan 2009 20:40:54 +0000 (+0000)
Subject: After HSH_Lookup() returns NULL indicating a busy object, we diddled
X-Git-Url: https://err.no/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c9d9a8b30a2319e6ec236fc105a1f09490c49650;p=varnish

After HSH_Lookup() returns NULL indicating a busy object, we diddled
the session a bit to transfer the per-request stats to the session
counters with SES_Charge().

Not only was it inconsistent to charge accounting data in the middle
of a request, it was also illegal because after the hash lock was
released we no longer owned the session.

Once a system is under sufficient load that there is a queue for the
CPU, a race could happen where upon hitting a busy object, the hash lock
was released, another thread would schedule, finish the busy object,
start the sessions on the waiting list, finish off the request we had
and then when we get the cpu again and access it, it's gone.

The previous commit (r3512) eliminated the need to call SES_Charge,
this commit removes the (option) shmlog message inside the hash lock
thus, hopefully, eliminating the race that caused #418.

Fixes: #418


git-svn-id: svn+ssh://projects.linpro.no/svn/varnish/trunk@3513 d4fa192b-c00b-0410-8231-f00ffab90ce4
---

diff --git a/varnish-cache/bin/varnishd/cache_center.c b/varnish-cache/bin/varnishd/cache_center.c
index 258fbeb2..f2445649 100644
--- a/varnish-cache/bin/varnishd/cache_center.c
+++ b/varnish-cache/bin/varnishd/cache_center.c
@@ -584,13 +584,10 @@ cnt_lookup(struct sess *sp)
 
 	if (o == NULL) {
 		/*
-		 * We hit a busy object, disembark worker thread and expect
-		 * hash code to restart us, still in STP_LOOKUP, later.
+		 * We lost the session to a busy object, disembark the
+		 * worker thread.   The hash code to restart the session,
+		 * still in STP_LOOKUP, later when the busy object isn't.
 		 */
-		assert(sp->objhead != NULL);
-		if (params->diag_bitmap & 0x20)
-			WSP(sp, SLT_Debug,
-			    "on waiting list <%s>", sp->objhead->hash);
 		return (1);
 	}
 
diff --git a/varnish-cache/bin/varnishd/cache_hash.c b/varnish-cache/bin/varnishd/cache_hash.c
index 112b41f2..20760bc0 100644
--- a/varnish-cache/bin/varnishd/cache_hash.c
+++ b/varnish-cache/bin/varnishd/cache_hash.c
@@ -298,6 +298,9 @@ HSH_Lookup(struct sess *sp)
 		/* There are one or more busy objects, wait for them */
 		if (sp->esis == 0)
 			VTAILQ_INSERT_TAIL(&oh->waitinglist, sp, list);
+		if (params->diag_bitmap & 0x20)
+			WSP(sp, SLT_Debug,
+				"on waiting list <%s>", oh->hash);
 		sp->objhead = oh;
 		Lck_Unlock(&oh->mtx);
 		return (NULL);