diff options
| -rw-r--r-- | source/cache.c | 37 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| -rwxr-xr-x | tests/t0020-validate-cache.sh | 29 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
2 files changed, 63 insertions, 3 deletions
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.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 |