error cleanup and slight change to destruction order

We now fini the root pesi while other tasks could still be running

error cleanup and slight change to destruction order
We now fini the root pesi while other tasks could still be running
de1fe5ee · Nils Goroll · 8aefe491 · de1fe5ee · de1fe5ee
Commit de1fe5ee authored Jul 30, 2019 by Nils Goroll
Hide whitespace changes
Inline Side-by-side

Showing with 68 additions and 8 deletions

client_close.vtc src/tests/client_close.vtc +60 -0

vdp_pesi.c src/vdp_pesi.c +8 -8

No files found.
--- a/src/tests/client_close.vtc
+++ b/src/tests/client_close.vtc
+varnishtest "ESI include - like e00003.vtc but early client close"
+
+
+server s1 {
+	rxreq
+	expect req.http.esi0 == "foo"
+	txresp -body {
+		<html>
+		Before include
+		<esi:include src="/body" sr="foo"/>
+		After include
+		</html>
+	}
+} -start
+
+server s2 {
+	rxreq
+	expect req.url == "/body1"
+	expect req.http.esi0 != "foo"
+	txresp -body {
+		Included file
+	}
+} -start
+
+varnish v1 -arg "-p debug=+syncvsl" -vcl+backend {
+	import ${vmod_pesi};
+
+	sub vcl_recv {
+		if (req.esi_level > 0) {
+			set req.url = req.url + req.esi_level;
+		} else {
+			set req.http.esi0 = "foo";
+		}
+	}
+
+	sub vcl_backend_fetch {
+		if (bereq.url == "/") {
+			set bereq.backend = s1;
+		}
+		else {
+			set bereq.backend = s2;
+		}
+	}
+
+	sub vcl_backend_response {
+		if (bereq.url == "/") {
+			set beresp.do_esi = true;
+		}
+	}
+
+	sub vcl_deliver {
+		pesi.activate();
+	}
+} -start
+
+client c1 {
+	txreq -hdr "Host: foo"
+	rxresphdrs
+	rxchunk
+} -run
--- a/src/vdp_pesi.c
+++ b/src/vdp_pesi.c
@@ -414,11 +414,7 @@ pesi_destroy(struct pesi **pesip)
 	Lck_Lock(&pesi_tree->task_lock);
 	VTAILQ_REMOVE(&pesi_tree->task_head, pesi, list);

-	/*
-	 * next assert is only true because the topreq happens to call
-	 * task_fini() after pesi_destroy()
-	 */
-	assert(pesi_tree->task_running > 0);
+	assert(pesi_tree->task_running >= 0);

 	if (pesi->has_task == 1)
 		pesi_tree->task_finishing++;
@@ -2080,9 +2076,6 @@ vdp_pesi_fini(struct req *req, void **priv)
 	CHECK_OBJ_NOTNULL(bytes_tree, BYTES_TREE_MAGIC);
 	assert(bytes_tree == pesi_tree->tree);

-	//// XXX needed?
-	//tree_fini(req, bytes_tree->root);
-
 	pesi_destroy(&pesi);
 	task_fini(pesi_tree, pesi);

@@ -2100,6 +2093,13 @@ vdp_pesi_fini(struct req *req, void **priv)
 		Lck_Unlock(&pesi_tree->task_lock);
 	}

+	/*
+	 * for an early client close, our subrequests had no chance to run
+	 *
+	 * we likely need more error handling
+	 */
+	tree_fini(req, bytes_tree->root);
+
 	AZ(pesi_tree->task_running);
 	assert(VTAILQ_EMPTY(&pesi_tree->task_head));
 	Lck_Delete(&pesi_tree->task_lock);