Serve nothing further once a fill is abandoned

2 files changed, 63 insertions, 3 deletions

diff --git a/source/cache.c b/source/cache.cindex 435cc96..95275fb 100644--- a/source/cache.c+++ b/source/cache.c@@ -55,6 +55,10 @@ struct cache_slot { 	const char *path; 	const char *lock_path; 	int key_matches;+	// Set when the fill was abandoned part way through, meaning the error+	// page has already reached the visitor and nothing more may be served+	// after it, not the lock file and not the stale copy still open.+	int abandoned; 	// The slot as it was when it was opened, or the lock file once 	// fill_slot has written a page into it. 	struct stat st;@@ -272,11 +276,14 @@ void cache_abandon_fill(void) 	if (!slot) 		return; 	slot_being_filled = NULL;+	slot->abandoned = 1; -	// Emptied while stdout still points at the lock file, so the half+	// The page is sitting in html.c's buffer and in stdio's, and both are+	// emptied while stdout still points at the lock file, so the half 	// rendered page goes into the file about to be removed rather than 	// reaching the client ahead of whatever is written next. 	html_flush();+	fflush(stdout);  	if (slot->saved_stdout >= 0) { 		dup2(slot->saved_stdout, STDOUT_FILENO);@@ -305,11 +312,15 @@ static int fill_slot(struct cache_slot *slot)  	// The page is sitting in html.c's buffer and then in stdio's, and all 	// of it has to reach the lock file before that file is renamed into-	// place.+	// place. After an abandoned fill stdout is the client again and this+	// same flush delivers the tail of the error page instead. 	html_flush(); 	if (fflush(stdout)) 		return errno; +	if (slot->abandoned)+		return 0;+ 	// print_slot takes the length of what it copies from here, and what 	// it copies after a fill is the lock file rather than the old slot. 	if (fstat(slot->lock_fd, &slot->st))@@ -334,6 +345,10 @@ static void refresh_slot(struct cache_slot *slot) 	if (is_modified(slot) || fill_slot(slot)) { 		unlock_slot(slot, 0); 		close_lock(slot);+	} else if (slot->abandoned) {+		// The abandoned fill answered the visitor itself and removed+		// the lock file, so only the descriptor is left to clean up.+		close_lock(slot); 	} else { 		close_slot(slot); 		unlock_slot(slot, 1);@@ -349,6 +364,13 @@ static int process_slot(struct cache_slot *slot) 	if (!err && slot->key_matches) { 		if (is_expired(slot)) 			refresh_slot(slot);+		// A refresh the error page abandoned has already answered the+		// visitor, and serving the stale copy still open would append+		// a second page to that answer.+		if (slot->abandoned) {+			close_slot(slot);+			return 0;+		} 		err = serve_slot(slot); 		close_slot(slot); 		return err;@@ -378,7 +400,15 @@ static int process_slot(struct cache_slot *slot) 			  slot->lock_path, strerror(err), err); 		unlock_slot(slot, 0); 		close_lock(slot);-		slot->fn();+		// Rendering again is only right when nothing was delivered,+		// and an abandoned fill has already sent the error page.+		if (!slot->abandoned)+			slot->fn();+		return 0;+	}++	if (slot->abandoned) {+		close_lock(slot); 		return 0; 	} @@ -466,6 +496,7 @@ int cache_process(int size, const char *path, const char *key, int ttl, 	slot.fn = fn; 	slot.ttl = ttl; 	slot.saved_stdout = -1;+	slot.abandoned = 0; 	slot.path = slot_path.buf; 	slot.lock_path = lock_path.buf; 	slot.key = key;
diff --git a/source/cache.c b/source/cache.c
index 435cc96..95275fb 100644
--- a/source/cache.c
+++ b/source/cache.c
@@ -55,6 +55,10 @@ struct cache_slot {
 	const char *path;
 	const char *lock_path;
 	int key_matches;
+	// Set when the fill was abandoned part way through, meaning the error
+	// page has already reached the visitor and nothing more may be served
+	// after it, not the lock file and not the stale copy still open.
+	int abandoned;
 	// The slot as it was when it was opened, or the lock file once
 	// fill_slot has written a page into it.
 	struct stat st;
@@ -272,11 +276,14 @@ void cache_abandon_fill(void)
 	if (!slot)
 		return;
 	slot_being_filled = NULL;
+	slot->abandoned = 1;
 
-	// Emptied while stdout still points at the lock file, so the half
+	// The page is sitting in html.c's buffer and in stdio's, and both are
+	// emptied while stdout still points at the lock file, so the half
 	// rendered page goes into the file about to be removed rather than
 	// reaching the client ahead of whatever is written next.
 	html_flush();
+	fflush(stdout);
 
 	if (slot->saved_stdout >= 0) {
 		dup2(slot->saved_stdout, STDOUT_FILENO);
@@ -305,11 +312,15 @@ static int fill_slot(struct cache_slot *slot)
 
 	// The page is sitting in html.c's buffer and then in stdio's, and all
 	// of it has to reach the lock file before that file is renamed into
-	// place.
+	// place. After an abandoned fill stdout is the client again and this
+	// same flush delivers the tail of the error page instead.
 	html_flush();
 	if (fflush(stdout))
 		return errno;
 
+	if (slot->abandoned)
+		return 0;
+
 	// print_slot takes the length of what it copies from here, and what
 	// it copies after a fill is the lock file rather than the old slot.
 	if (fstat(slot->lock_fd, &slot->st))
@@ -334,6 +345,10 @@ static void refresh_slot(struct cache_slot *slot)
 	if (is_modified(slot) || fill_slot(slot)) {
 		unlock_slot(slot, 0);
 		close_lock(slot);
+	} else if (slot->abandoned) {
+		// The abandoned fill answered the visitor itself and removed
+		// the lock file, so only the descriptor is left to clean up.
+		close_lock(slot);
 	} else {
 		close_slot(slot);
 		unlock_slot(slot, 1);
@@ -349,6 +364,13 @@ static int process_slot(struct cache_slot *slot)
 	if (!err && slot->key_matches) {
 		if (is_expired(slot))
 			refresh_slot(slot);
+		// A refresh the error page abandoned has already answered the
+		// visitor, and serving the stale copy still open would append
+		// a second page to that answer.
+		if (slot->abandoned) {
+			close_slot(slot);
+			return 0;
+		}
 		err = serve_slot(slot);
 		close_slot(slot);
 		return err;
@@ -378,7 +400,15 @@ static int process_slot(struct cache_slot *slot)
 			  slot->lock_path, strerror(err), err);
 		unlock_slot(slot, 0);
 		close_lock(slot);
-		slot->fn();
+		// Rendering again is only right when nothing was delivered,
+		// and an abandoned fill has already sent the error page.
+		if (!slot->abandoned)
+			slot->fn();
+		return 0;
+	}
+
+	if (slot->abandoned) {
+		close_lock(slot);
 		return 0;
 	}
 
@@ -466,6 +496,7 @@ int cache_process(int size, const char *path, const char *key, int ttl,
 	slot.fn = fn;
 	slot.ttl = ttl;
 	slot.saved_stdout = -1;
+	slot.abandoned = 0;
 	slot.path = slot_path.buf;
 	slot.lock_path = lock_path.buf;
 	slot.key = key;
diff --git a/tests/t0020-validate-cache.sh b/tests/t0020-validate-cache.shindex feb6275..ca85d97 100755--- a/tests/t0020-validate-cache.sh+++ b/tests/t0020-validate-cache.sh@@ -154,6 +154,35 @@ test_expect_success 'an ordinary key still fills a slot' ' 	test_line_count = 1 key.slots ' +# An error page fired after rendering began abandons the fill with part of+# the page already written into the lock file. That fragment must be+# discarded along with the file, not replayed to the visitor behind the+# error page. A commit whose parent object is missing renders its whole+# info table before the diff machinery fails, which makes it the trigger.+test_expect_success 'set up a repo missing a parent object' '+	mkrepo repos/broken 2 &&+	parent=$(git -C repos/broken rev-parse HEAD^) &&+	tip=$(git -C repos/broken rev-parse HEAD) &&+	rm "repos/broken/.git/objects/$(echo "$parent" | cut -c1-2)/$(echo "$parent" | cut -c3-)" &&+	{+		echo "virtual-root=/" &&+		echo "cache-root=$PWD/cache4" &&+		echo "cache-size=64" &&+		echo "repo.url=broken" &&+		echo "repo.path=$PWD/repos/broken/.git"+	} >brokenrc &&+	rm -rf cache4 && mkdir cache4+'++test_expect_success 'an error after output began replays none of it' '+	CGIT_CONFIG="$PWD/brokenrc" QUERY_STRING="url=broken/commit/&id=$tip" \+	cgit >broken.out &&+	grep "Bad commit" broken.out &&+	test $(grep -c "^Status:" broken.out) = 1 &&+	ls cache4 >broken.slots &&+	test_line_count = 0 broken.slots+'+ # An error page reports a condition the repository may grow out of, and a # request pinned to an object id would cache it under the never-expiring # static ttl. Asking for a commit that does not exist yet must therefore
diff --git a/tests/t0020-validate-cache.sh b/tests/t0020-validate-cache.sh
index feb6275..ca85d97 100755
--- a/tests/t0020-validate-cache.sh
+++ b/tests/t0020-validate-cache.sh
@@ -154,6 +154,35 @@ test_expect_success 'an ordinary key still fills a slot' '
 	test_line_count = 1 key.slots
 '
 
+# An error page fired after rendering began abandons the fill with part of
+# the page already written into the lock file. That fragment must be
+# discarded along with the file, not replayed to the visitor behind the
+# error page. A commit whose parent object is missing renders its whole
+# info table before the diff machinery fails, which makes it the trigger.
+test_expect_success 'set up a repo missing a parent object' '
+	mkrepo repos/broken 2 &&
+	parent=$(git -C repos/broken rev-parse HEAD^) &&
+	tip=$(git -C repos/broken rev-parse HEAD) &&
+	rm "repos/broken/.git/objects/$(echo "$parent" | cut -c1-2)/$(echo "$parent" | cut -c3-)" &&
+	{
+		echo "virtual-root=/" &&
+		echo "cache-root=$PWD/cache4" &&
+		echo "cache-size=64" &&
+		echo "repo.url=broken" &&
+		echo "repo.path=$PWD/repos/broken/.git"
+	} >brokenrc &&
+	rm -rf cache4 && mkdir cache4
+'
+
+test_expect_success 'an error after output began replays none of it' '
+	CGIT_CONFIG="$PWD/brokenrc" QUERY_STRING="url=broken/commit/&id=$tip" \
+	cgit >broken.out &&
+	grep "Bad commit" broken.out &&
+	test $(grep -c "^Status:" broken.out) = 1 &&
+	ls cache4 >broken.slots &&
+	test_line_count = 0 broken.slots
+'
+
 # An error page reports a condition the repository may grow out of, and a
 # request pinned to an object id would cache it under the never-expiring
 # static ttl. Asking for a commit that does not exist yet must therefore