From 64ac8440a7021863496bc7eb10dc9f5e8cc44727 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Mon, 4 May 2020 16:46:11 -0500 Subject: [PATCH 01/42] Make archival asynchronous The prime benefit being sought here is for large archives to not clog up the rendering process and cause unsightly proxy timeouts. As a secondary benefit, archive-in-progress is moved out of the way into a /tmp file so that new archival requests for the same commit will not get fulfilled based on an archive that isn't yet finished. This asynchronous system is fairly primitive; request comes in, we'll spawn off a new goroutine to handle it, then we'll mark it as done. Status requests will see if the file exists in the final location, and report the archival as done when it exists. Fixes #11265 --- integrations/api_repo_test.go | 6 +- .../user27/repo49.git/HEAD | 1 + .../user27/repo49.git/config | 6 + .../user27/repo49.git/description | 1 + .../repo49.git/hooks/applypatch-msg.sample | 15 ++ .../user27/repo49.git/hooks/commit-msg.sample | 24 ++ .../hooks/fsmonitor-watchman.sample | 114 ++++++++ .../user27/repo49.git/hooks/post-receive | 15 ++ .../repo49.git/hooks/post-receive.d/gitea | 2 + .../repo49.git/hooks/post-update.sample | 8 + .../repo49.git/hooks/pre-applypatch.sample | 14 + .../user27/repo49.git/hooks/pre-commit.sample | 49 ++++ .../user27/repo49.git/hooks/pre-push.sample | 53 ++++ .../user27/repo49.git/hooks/pre-rebase.sample | 169 ++++++++++++ .../user27/repo49.git/hooks/pre-receive | 15 ++ .../repo49.git/hooks/pre-receive.d/gitea | 2 + .../repo49.git/hooks/pre-receive.sample | 24 ++ .../hooks/prepare-commit-msg.sample | 42 +++ .../user27/repo49.git/hooks/update | 14 + .../user27/repo49.git/hooks/update.d/gitea | 2 + .../user27/repo49.git/hooks/update.sample | 128 +++++++++ .../user27/repo49.git/info/exclude | 6 + .../user27/repo49.git/info/refs | 1 + .../47/34b1f84a367fa1b81c31aa4234a5bad11cafa3 | Bin 0 -> 84 bytes .../4d/31f3a12656368a8d9180f431d40d0fc408be2d | Bin 0 -> 29 bytes .../51/f84af231345367fd5d61ceb89efb3b6d757061 | Bin 0 -> 121 bytes .../79/3aa682b06ae032641abf70c5dfeade28c07c52 | Bin 0 -> 28 bytes .../aa/cbdfe9e1c4b47f60abe81849045fa4e96f1d75 | Bin 0 -> 154 bytes .../dd/392e939ea4936b2459219c9c9a1f25547ccaeb | Bin 0 -> 53 bytes .../f2/8eeca3df7614fd4f10c1030f13feb418ef3c6f | Bin 0 -> 54 bytes .../user27/repo49.git/objects/info/packs | 1 + .../user27/repo49.git/refs/heads/master | 1 + models/fixtures/repository.yml | 13 + models/fixtures/user.yml | 2 +- models/repo_list_test.go | 6 +- routers/repo/repo.go | 107 +++----- routers/routes/routes.go | 6 +- services/archiver/archiver.go | 250 ++++++++++++++++++ services/archiver/archiver_test.go | 103 ++++++++ templates/repo/branch/list.tmpl | 8 +- templates/repo/home.tmpl | 4 +- templates/repo/release/list.tmpl | 8 +- web_src/js/index.js | 81 ++++++ 43 files changed, 1216 insertions(+), 85 deletions(-) create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/HEAD create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/config create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/description create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/applypatch-msg.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/commit-msg.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/fsmonitor-watchman.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-receive create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-receive.d/gitea create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-update.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-applypatch.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-commit.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-push.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-rebase.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive.d/gitea create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/prepare-commit-msg.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/update create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/update.d/gitea create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/hooks/update.sample create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/info/exclude create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/info/refs create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/objects/47/34b1f84a367fa1b81c31aa4234a5bad11cafa3 create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/objects/4d/31f3a12656368a8d9180f431d40d0fc408be2d create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/objects/51/f84af231345367fd5d61ceb89efb3b6d757061 create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/objects/79/3aa682b06ae032641abf70c5dfeade28c07c52 create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/objects/aa/cbdfe9e1c4b47f60abe81849045fa4e96f1d75 create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/objects/dd/392e939ea4936b2459219c9c9a1f25547ccaeb create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/objects/f2/8eeca3df7614fd4f10c1030f13feb418ef3c6f create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/objects/info/packs create mode 100644 integrations/gitea-repositories-meta/user27/repo49.git/refs/heads/master create mode 100644 services/archiver/archiver.go create mode 100644 services/archiver/archiver_test.go diff --git a/integrations/api_repo_test.go b/integrations/api_repo_test.go index d57e5daaf3e5b..cf46567f66d87 100644 --- a/integrations/api_repo_test.go +++ b/integrations/api_repo_test.go @@ -70,9 +70,9 @@ func TestAPISearchRepo(t *testing.T) { expectedResults }{ {name: "RepositoriesMax50", requestURL: "/api/v1/repos/search?limit=50&private=false", expectedResults: expectedResults{ - nil: {count: 27}, - user: {count: 27}, - user2: {count: 27}}, + nil: {count: 28}, + user: {count: 28}, + user2: {count: 28}}, }, {name: "RepositoriesMax10", requestURL: "/api/v1/repos/search?limit=10&private=false", expectedResults: expectedResults{ nil: {count: 10}, diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/HEAD b/integrations/gitea-repositories-meta/user27/repo49.git/HEAD new file mode 100644 index 0000000000000..cb089cd89a7d7 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/HEAD @@ -0,0 +1 @@ +ref: refs/heads/master diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/config b/integrations/gitea-repositories-meta/user27/repo49.git/config new file mode 100644 index 0000000000000..64280b806c976 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/config @@ -0,0 +1,6 @@ +[core] + repositoryformatversion = 0 + filemode = false + bare = true + symlinks = false + ignorecase = true diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/description b/integrations/gitea-repositories-meta/user27/repo49.git/description new file mode 100644 index 0000000000000..498b267a8c781 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/description @@ -0,0 +1 @@ +Unnamed repository; edit this file 'description' to name the repository. diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/applypatch-msg.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/applypatch-msg.sample new file mode 100644 index 0000000000000..a5d7b84a67345 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/applypatch-msg.sample @@ -0,0 +1,15 @@ +#!/bin/sh +# +# An example hook script to check the commit log message taken by +# applypatch from an e-mail message. +# +# The hook should exit with non-zero status after issuing an +# appropriate message if it wants to stop the commit. The hook is +# allowed to edit the commit message file. +# +# To enable this hook, rename this file to "applypatch-msg". + +. git-sh-setup +commitmsg="$(git rev-parse --git-path hooks/commit-msg)" +test -x "$commitmsg" && exec "$commitmsg" ${1+"$@"} +: diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/commit-msg.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/commit-msg.sample new file mode 100644 index 0000000000000..b58d1184a9d43 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/commit-msg.sample @@ -0,0 +1,24 @@ +#!/bin/sh +# +# An example hook script to check the commit log message. +# Called by "git commit" with one argument, the name of the file +# that has the commit message. The hook should exit with non-zero +# status after issuing an appropriate message if it wants to stop the +# commit. The hook is allowed to edit the commit message file. +# +# To enable this hook, rename this file to "commit-msg". + +# Uncomment the below to add a Signed-off-by line to the message. +# Doing this in a hook is a bad idea in general, but the prepare-commit-msg +# hook is more suited to it. +# +# SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p') +# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1" + +# This example catches duplicate Signed-off-by lines. + +test "" = "$(grep '^Signed-off-by: ' "$1" | + sort | uniq -c | sed -e '/^[ ]*1[ ]/d')" || { + echo >&2 Duplicate Signed-off-by lines. + exit 1 +} diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/fsmonitor-watchman.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/fsmonitor-watchman.sample new file mode 100644 index 0000000000000..e673bb3980f3c --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/fsmonitor-watchman.sample @@ -0,0 +1,114 @@ +#!/usr/bin/perl + +use strict; +use warnings; +use IPC::Open2; + +# An example hook script to integrate Watchman +# (https://facebook.github.io/watchman/) with git to speed up detecting +# new and modified files. +# +# The hook is passed a version (currently 1) and a time in nanoseconds +# formatted as a string and outputs to stdout all files that have been +# modified since the given time. Paths must be relative to the root of +# the working tree and separated by a single NUL. +# +# To enable this hook, rename this file to "query-watchman" and set +# 'git config core.fsmonitor .git/hooks/query-watchman' +# +my ($version, $time) = @ARGV; + +# Check the hook interface version + +if ($version == 1) { + # convert nanoseconds to seconds + $time = int $time / 1000000000; +} else { + die "Unsupported query-fsmonitor hook version '$version'.\n" . + "Falling back to scanning...\n"; +} + +my $git_work_tree; +if ($^O =~ 'msys' || $^O =~ 'cygwin') { + $git_work_tree = Win32::GetCwd(); + $git_work_tree =~ tr/\\/\//; +} else { + require Cwd; + $git_work_tree = Cwd::cwd(); +} + +my $retry = 1; + +launch_watchman(); + +sub launch_watchman { + + my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty') + or die "open2() failed: $!\n" . + "Falling back to scanning...\n"; + + # In the query expression below we're asking for names of files that + # changed since $time but were not transient (ie created after + # $time but no longer exist). + # + # To accomplish this, we're using the "since" generator to use the + # recency index to select candidate nodes and "fields" to limit the + # output to file names only. Then we're using the "expression" term to + # further constrain the results. + # + # The category of transient files that we want to ignore will have a + # creation clock (cclock) newer than $time_t value and will also not + # currently exist. + + my $query = <<" END"; + ["query", "$git_work_tree", { + "since": $time, + "fields": ["name"], + "expression": ["not", ["allof", ["since", $time, "cclock"], ["not", "exists"]]] + }] + END + + print CHLD_IN $query; + close CHLD_IN; + my $response = do {local $/; }; + + die "Watchman: command returned no output.\n" . + "Falling back to scanning...\n" if $response eq ""; + die "Watchman: command returned invalid output: $response\n" . + "Falling back to scanning...\n" unless $response =~ /^\{/; + + my $json_pkg; + eval { + require JSON::XS; + $json_pkg = "JSON::XS"; + 1; + } or do { + require JSON::PP; + $json_pkg = "JSON::PP"; + }; + + my $o = $json_pkg->new->utf8->decode($response); + + if ($retry > 0 and $o->{error} and $o->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) { + print STDERR "Adding '$git_work_tree' to watchman's watch list.\n"; + $retry--; + qx/watchman watch "$git_work_tree"/; + die "Failed to make watchman watch '$git_work_tree'.\n" . + "Falling back to scanning...\n" if $? != 0; + + # Watchman will always return all files on the first query so + # return the fast "everything is dirty" flag to git and do the + # Watchman query just to get it over with now so we won't pay + # the cost in git to look up each individual file. + print "/\0"; + eval { launch_watchman() }; + exit 0; + } + + die "Watchman: $o->{error}.\n" . + "Falling back to scanning...\n" if $o->{error}; + + binmode STDOUT, ":utf8"; + local $, = "\0"; + print @{$o->{files}}; +} diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-receive b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-receive new file mode 100644 index 0000000000000..f1f2709dddeea --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-receive @@ -0,0 +1,15 @@ +#!/usr/bin/env bash +data=$(cat) +exitcodes="" +hookname=$(basename $0) +GIT_DIR=${GIT_DIR:-$(dirname $0)} + +for hook in ${GIT_DIR}/hooks/${hookname}.d/*; do +test -x "${hook}" && test -f "${hook}" || continue +echo "${data}" | "${hook}" +exitcodes="${exitcodes} $?" +done + +for i in ${exitcodes}; do +[ ${i} -eq 0 ] || exit ${i} +done diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-receive.d/gitea b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-receive.d/gitea new file mode 100644 index 0000000000000..43a948da3a983 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-receive.d/gitea @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +"$GITEA_ROOT/gitea" hook --config="$GITEA_ROOT/$GITEA_CONF" post-receive diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-update.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-update.sample new file mode 100644 index 0000000000000..ec17ec1939b7c --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/post-update.sample @@ -0,0 +1,8 @@ +#!/bin/sh +# +# An example hook script to prepare a packed repository for use over +# dumb transports. +# +# To enable this hook, rename this file to "post-update". + +exec git update-server-info diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-applypatch.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-applypatch.sample new file mode 100644 index 0000000000000..4142082bcb939 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-applypatch.sample @@ -0,0 +1,14 @@ +#!/bin/sh +# +# An example hook script to verify what is about to be committed +# by applypatch from an e-mail message. +# +# The hook should exit with non-zero status after issuing an +# appropriate message if it wants to stop the commit. +# +# To enable this hook, rename this file to "pre-applypatch". + +. git-sh-setup +precommit="$(git rev-parse --git-path hooks/pre-commit)" +test -x "$precommit" && exec "$precommit" ${1+"$@"} +: diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-commit.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-commit.sample new file mode 100644 index 0000000000000..6a756416384c2 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-commit.sample @@ -0,0 +1,49 @@ +#!/bin/sh +# +# An example hook script to verify what is about to be committed. +# Called by "git commit" with no arguments. The hook should +# exit with non-zero status after issuing an appropriate message if +# it wants to stop the commit. +# +# To enable this hook, rename this file to "pre-commit". + +if git rev-parse --verify HEAD >/dev/null 2>&1 +then + against=HEAD +else + # Initial commit: diff against an empty tree object + against=$(git hash-object -t tree /dev/null) +fi + +# If you want to allow non-ASCII filenames set this variable to true. +allownonascii=$(git config --bool hooks.allownonascii) + +# Redirect output to stderr. +exec 1>&2 + +# Cross platform projects tend to avoid non-ASCII filenames; prevent +# them from being added to the repository. We exploit the fact that the +# printable range starts at the space character and ends with tilde. +if [ "$allownonascii" != "true" ] && + # Note that the use of brackets around a tr range is ok here, (it's + # even required, for portability to Solaris 10's /usr/bin/tr), since + # the square bracket bytes happen to fall in the designated range. + test $(git diff --cached --name-only --diff-filter=A -z $against | + LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0 +then + cat <<\EOF +Error: Attempt to add a non-ASCII file name. + +This can cause problems if you want to work with people on other platforms. + +To be portable it is advisable to rename the file. + +If you know what you are doing you can disable this check using: + + git config hooks.allownonascii true +EOF + exit 1 +fi + +# If there are whitespace errors, print the offending file names and fail. +exec git diff-index --check --cached $against -- diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-push.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-push.sample new file mode 100644 index 0000000000000..6187dbf4390fc --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-push.sample @@ -0,0 +1,53 @@ +#!/bin/sh + +# An example hook script to verify what is about to be pushed. Called by "git +# push" after it has checked the remote status, but before anything has been +# pushed. If this script exits with a non-zero status nothing will be pushed. +# +# This hook is called with the following parameters: +# +# $1 -- Name of the remote to which the push is being done +# $2 -- URL to which the push is being done +# +# If pushing without using a named remote those arguments will be equal. +# +# Information about the commits which are being pushed is supplied as lines to +# the standard input in the form: +# +# +# +# This sample shows how to prevent push of commits where the log message starts +# with "WIP" (work in progress). + +remote="$1" +url="$2" + +z40=0000000000000000000000000000000000000000 + +while read local_ref local_sha remote_ref remote_sha +do + if [ "$local_sha" = $z40 ] + then + # Handle delete + : + else + if [ "$remote_sha" = $z40 ] + then + # New branch, examine all commits + range="$local_sha" + else + # Update to existing branch, examine new commits + range="$remote_sha..$local_sha" + fi + + # Check for WIP commit + commit=`git rev-list -n 1 --grep '^WIP' "$range"` + if [ -n "$commit" ] + then + echo >&2 "Found WIP commit in $local_ref, not pushing" + exit 1 + fi + fi +done + +exit 0 diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-rebase.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-rebase.sample new file mode 100644 index 0000000000000..6cbef5c370d8c --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-rebase.sample @@ -0,0 +1,169 @@ +#!/bin/sh +# +# Copyright (c) 2006, 2008 Junio C Hamano +# +# The "pre-rebase" hook is run just before "git rebase" starts doing +# its job, and can prevent the command from running by exiting with +# non-zero status. +# +# The hook is called with the following parameters: +# +# $1 -- the upstream the series was forked from. +# $2 -- the branch being rebased (or empty when rebasing the current branch). +# +# This sample shows how to prevent topic branches that are already +# merged to 'next' branch from getting rebased, because allowing it +# would result in rebasing already published history. + +publish=next +basebranch="$1" +if test "$#" = 2 +then + topic="refs/heads/$2" +else + topic=`git symbolic-ref HEAD` || + exit 0 ;# we do not interrupt rebasing detached HEAD +fi + +case "$topic" in +refs/heads/??/*) + ;; +*) + exit 0 ;# we do not interrupt others. + ;; +esac + +# Now we are dealing with a topic branch being rebased +# on top of master. Is it OK to rebase it? + +# Does the topic really exist? +git show-ref -q "$topic" || { + echo >&2 "No such branch $topic" + exit 1 +} + +# Is topic fully merged to master? +not_in_master=`git rev-list --pretty=oneline ^master "$topic"` +if test -z "$not_in_master" +then + echo >&2 "$topic is fully merged to master; better remove it." + exit 1 ;# we could allow it, but there is no point. +fi + +# Is topic ever merged to next? If so you should not be rebasing it. +only_next_1=`git rev-list ^master "^$topic" ${publish} | sort` +only_next_2=`git rev-list ^master ${publish} | sort` +if test "$only_next_1" = "$only_next_2" +then + not_in_topic=`git rev-list "^$topic" master` + if test -z "$not_in_topic" + then + echo >&2 "$topic is already up to date with master" + exit 1 ;# we could allow it, but there is no point. + else + exit 0 + fi +else + not_in_next=`git rev-list --pretty=oneline ^${publish} "$topic"` + /usr/bin/perl -e ' + my $topic = $ARGV[0]; + my $msg = "* $topic has commits already merged to public branch:\n"; + my (%not_in_next) = map { + /^([0-9a-f]+) /; + ($1 => 1); + } split(/\n/, $ARGV[1]); + for my $elem (map { + /^([0-9a-f]+) (.*)$/; + [$1 => $2]; + } split(/\n/, $ARGV[2])) { + if (!exists $not_in_next{$elem->[0]}) { + if ($msg) { + print STDERR $msg; + undef $msg; + } + print STDERR " $elem->[1]\n"; + } + } + ' "$topic" "$not_in_next" "$not_in_master" + exit 1 +fi + +<<\DOC_END + +This sample hook safeguards topic branches that have been +published from being rewound. + +The workflow assumed here is: + + * Once a topic branch forks from "master", "master" is never + merged into it again (either directly or indirectly). + + * Once a topic branch is fully cooked and merged into "master", + it is deleted. If you need to build on top of it to correct + earlier mistakes, a new topic branch is created by forking at + the tip of the "master". This is not strictly necessary, but + it makes it easier to keep your history simple. + + * Whenever you need to test or publish your changes to topic + branches, merge them into "next" branch. + +The script, being an example, hardcodes the publish branch name +to be "next", but it is trivial to make it configurable via +$GIT_DIR/config mechanism. + +With this workflow, you would want to know: + +(1) ... if a topic branch has ever been merged to "next". Young + topic branches can have stupid mistakes you would rather + clean up before publishing, and things that have not been + merged into other branches can be easily rebased without + affecting other people. But once it is published, you would + not want to rewind it. + +(2) ... if a topic branch has been fully merged to "master". + Then you can delete it. More importantly, you should not + build on top of it -- other people may already want to + change things related to the topic as patches against your + "master", so if you need further changes, it is better to + fork the topic (perhaps with the same name) afresh from the + tip of "master". + +Let's look at this example: + + o---o---o---o---o---o---o---o---o---o "next" + / / / / + / a---a---b A / / + / / / / + / / c---c---c---c B / + / / / \ / + / / / b---b C \ / + / / / / \ / + ---o---o---o---o---o---o---o---o---o---o---o "master" + + +A, B and C are topic branches. + + * A has one fix since it was merged up to "next". + + * B has finished. It has been fully merged up to "master" and "next", + and is ready to be deleted. + + * C has not merged to "next" at all. + +We would want to allow C to be rebased, refuse A, and encourage +B to be deleted. + +To compute (1): + + git rev-list ^master ^topic next + git rev-list ^master next + + if these match, topic has not merged in next at all. + +To compute (2): + + git rev-list master..topic + + if this is empty, it is fully merged to "master". + +DOC_END diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive new file mode 100644 index 0000000000000..f1f2709dddeea --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive @@ -0,0 +1,15 @@ +#!/usr/bin/env bash +data=$(cat) +exitcodes="" +hookname=$(basename $0) +GIT_DIR=${GIT_DIR:-$(dirname $0)} + +for hook in ${GIT_DIR}/hooks/${hookname}.d/*; do +test -x "${hook}" && test -f "${hook}" || continue +echo "${data}" | "${hook}" +exitcodes="${exitcodes} $?" +done + +for i in ${exitcodes}; do +[ ${i} -eq 0 ] || exit ${i} +done diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive.d/gitea b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive.d/gitea new file mode 100644 index 0000000000000..49d09406364a5 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive.d/gitea @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +"$GITEA_ROOT/gitea" hook --config="$GITEA_ROOT/$GITEA_CONF" pre-receive diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive.sample new file mode 100644 index 0000000000000..a1fd29ec14823 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/pre-receive.sample @@ -0,0 +1,24 @@ +#!/bin/sh +# +# An example hook script to make use of push options. +# The example simply echoes all push options that start with 'echoback=' +# and rejects all pushes when the "reject" push option is used. +# +# To enable this hook, rename this file to "pre-receive". + +if test -n "$GIT_PUSH_OPTION_COUNT" +then + i=0 + while test "$i" -lt "$GIT_PUSH_OPTION_COUNT" + do + eval "value=\$GIT_PUSH_OPTION_$i" + case "$value" in + echoback=*) + echo "echo from the pre-receive-hook: ${value#*=}" >&2 + ;; + reject) + exit 1 + esac + i=$((i + 1)) + done +fi diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/prepare-commit-msg.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/prepare-commit-msg.sample new file mode 100644 index 0000000000000..10fa14c5ab013 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/prepare-commit-msg.sample @@ -0,0 +1,42 @@ +#!/bin/sh +# +# An example hook script to prepare the commit log message. +# Called by "git commit" with the name of the file that has the +# commit message, followed by the description of the commit +# message's source. The hook's purpose is to edit the commit +# message file. If the hook fails with a non-zero status, +# the commit is aborted. +# +# To enable this hook, rename this file to "prepare-commit-msg". + +# This hook includes three examples. The first one removes the +# "# Please enter the commit message..." help message. +# +# The second includes the output of "git diff --name-status -r" +# into the message, just before the "git status" output. It is +# commented because it doesn't cope with --amend or with squashed +# commits. +# +# The third example adds a Signed-off-by line to the message, that can +# still be edited. This is rarely a good idea. + +COMMIT_MSG_FILE=$1 +COMMIT_SOURCE=$2 +SHA1=$3 + +/usr/bin/perl -i.bak -ne 'print unless(m/^. Please enter the commit message/..m/^#$/)' "$COMMIT_MSG_FILE" + +# case "$COMMIT_SOURCE,$SHA1" in +# ,|template,) +# /usr/bin/perl -i.bak -pe ' +# print "\n" . `git diff --cached --name-status -r` +# if /^#/ && $first++ == 0' "$COMMIT_MSG_FILE" ;; +# *) ;; +# esac + +# SOB=$(git var GIT_COMMITTER_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p') +# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE" +# if test -z "$COMMIT_SOURCE" +# then +# /usr/bin/perl -i.bak -pe 'print "\n" if !$first_line++' "$COMMIT_MSG_FILE" +# fi diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/update b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/update new file mode 100644 index 0000000000000..df5bd27f106f2 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/update @@ -0,0 +1,14 @@ +#!/usr/bin/env bash +exitcodes="" +hookname=$(basename $0) +GIT_DIR=${GIT_DIR:-$(dirname $0)} + +for hook in ${GIT_DIR}/hooks/${hookname}.d/*; do +test -x "${hook}" && test -f "${hook}" || continue +"${hook}" $1 $2 $3 +exitcodes="${exitcodes} $?" +done + +for i in ${exitcodes}; do +[ ${i} -eq 0 ] || exit ${i} +done diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/update.d/gitea b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/update.d/gitea new file mode 100644 index 0000000000000..38101c242664a --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/update.d/gitea @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +"$GITEA_ROOT/gitea" hook --config="$GITEA_ROOT/$GITEA_CONF" update $1 $2 $3 diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/hooks/update.sample b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/update.sample new file mode 100644 index 0000000000000..80ba94135cc37 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/hooks/update.sample @@ -0,0 +1,128 @@ +#!/bin/sh +# +# An example hook script to block unannotated tags from entering. +# Called by "git receive-pack" with arguments: refname sha1-old sha1-new +# +# To enable this hook, rename this file to "update". +# +# Config +# ------ +# hooks.allowunannotated +# This boolean sets whether unannotated tags will be allowed into the +# repository. By default they won't be. +# hooks.allowdeletetag +# This boolean sets whether deleting tags will be allowed in the +# repository. By default they won't be. +# hooks.allowmodifytag +# This boolean sets whether a tag may be modified after creation. By default +# it won't be. +# hooks.allowdeletebranch +# This boolean sets whether deleting branches will be allowed in the +# repository. By default they won't be. +# hooks.denycreatebranch +# This boolean sets whether remotely creating branches will be denied +# in the repository. By default this is allowed. +# + +# --- Command line +refname="$1" +oldrev="$2" +newrev="$3" + +# --- Safety check +if [ -z "$GIT_DIR" ]; then + echo "Don't run this script from the command line." >&2 + echo " (if you want, you could supply GIT_DIR then run" >&2 + echo " $0 )" >&2 + exit 1 +fi + +if [ -z "$refname" -o -z "$oldrev" -o -z "$newrev" ]; then + echo "usage: $0 " >&2 + exit 1 +fi + +# --- Config +allowunannotated=$(git config --bool hooks.allowunannotated) +allowdeletebranch=$(git config --bool hooks.allowdeletebranch) +denycreatebranch=$(git config --bool hooks.denycreatebranch) +allowdeletetag=$(git config --bool hooks.allowdeletetag) +allowmodifytag=$(git config --bool hooks.allowmodifytag) + +# check for no description +projectdesc=$(sed -e '1q' "$GIT_DIR/description") +case "$projectdesc" in +"Unnamed repository"* | "") + echo "*** Project description file hasn't been set" >&2 + exit 1 + ;; +esac + +# --- Check types +# if $newrev is 0000...0000, it's a commit to delete a ref. +zero="0000000000000000000000000000000000000000" +if [ "$newrev" = "$zero" ]; then + newrev_type=delete +else + newrev_type=$(git cat-file -t $newrev) +fi + +case "$refname","$newrev_type" in + refs/tags/*,commit) + # un-annotated tag + short_refname=${refname##refs/tags/} + if [ "$allowunannotated" != "true" ]; then + echo "*** The un-annotated tag, $short_refname, is not allowed in this repository" >&2 + echo "*** Use 'git tag [ -a | -s ]' for tags you want to propagate." >&2 + exit 1 + fi + ;; + refs/tags/*,delete) + # delete tag + if [ "$allowdeletetag" != "true" ]; then + echo "*** Deleting a tag is not allowed in this repository" >&2 + exit 1 + fi + ;; + refs/tags/*,tag) + # annotated tag + if [ "$allowmodifytag" != "true" ] && git rev-parse $refname > /dev/null 2>&1 + then + echo "*** Tag '$refname' already exists." >&2 + echo "*** Modifying a tag is not allowed in this repository." >&2 + exit 1 + fi + ;; + refs/heads/*,commit) + # branch + if [ "$oldrev" = "$zero" -a "$denycreatebranch" = "true" ]; then + echo "*** Creating a branch is not allowed in this repository" >&2 + exit 1 + fi + ;; + refs/heads/*,delete) + # delete branch + if [ "$allowdeletebranch" != "true" ]; then + echo "*** Deleting a branch is not allowed in this repository" >&2 + exit 1 + fi + ;; + refs/remotes/*,commit) + # tracking branch + ;; + refs/remotes/*,delete) + # delete tracking branch + if [ "$allowdeletebranch" != "true" ]; then + echo "*** Deleting a tracking branch is not allowed in this repository" >&2 + exit 1 + fi + ;; + *) + # Anything else (is there anything else?) + echo "*** Update hook: unknown type of update to ref $refname of type $newrev_type" >&2 + exit 1 + ;; +esac + +# --- Finished +exit 0 diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/info/exclude b/integrations/gitea-repositories-meta/user27/repo49.git/info/exclude new file mode 100644 index 0000000000000..a5196d1be8fb5 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/info/exclude @@ -0,0 +1,6 @@ +# git ls-files --others --exclude-from=.git/info/exclude +# Lines that start with '#' are comments. +# For a project mostly in C, the following would be a good set of +# exclude patterns (uncomment them if you want to use them): +# *.[oa] +# *~ diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/info/refs b/integrations/gitea-repositories-meta/user27/repo49.git/info/refs new file mode 100644 index 0000000000000..22f08279c0d70 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/info/refs @@ -0,0 +1 @@ +aacbdfe9e1c4b47f60abe81849045fa4e96f1d75 refs/heads/master diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/objects/47/34b1f84a367fa1b81c31aa4234a5bad11cafa3 b/integrations/gitea-repositories-meta/user27/repo49.git/objects/47/34b1f84a367fa1b81c31aa4234a5bad11cafa3 new file mode 100644 index 0000000000000000000000000000000000000000..b6f121a4bb5d3bef8b829a9b56eb50fd3dcf8fce GIT binary patch literal 84 zcmV-a0IUCa0V^p=O;s?nU@$Z=Ff%bx2y%6F@paY9O=0jg{Jc;t%&e<-V#621E4=(i qIQHq97yyAnNosKk!(B_g$@7*>&Q^(3oHJ*ZylP0zsn-Cegc}#|fhG+A literal 0 HcmV?d00001 diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/objects/4d/31f3a12656368a8d9180f431d40d0fc408be2d b/integrations/gitea-repositories-meta/user27/repo49.git/objects/4d/31f3a12656368a8d9180f431d40d0fc408be2d new file mode 100644 index 0000000000000000000000000000000000000000..d2f4c1d04ed06ac5c7caf9aca1f44840f06a5f81 GIT binary patch literal 29 kcmblhEoiK()F0GTUJQ8B?D`S@e0H0(Eg8%>k literal 0 HcmV?d00001 diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/objects/aa/cbdfe9e1c4b47f60abe81849045fa4e96f1d75 b/integrations/gitea-repositories-meta/user27/repo49.git/objects/aa/cbdfe9e1c4b47f60abe81849045fa4e96f1d75 new file mode 100644 index 0000000000000000000000000000000000000000..74419f4b47cf77881c7d2b8f6716c5162c0c1584 GIT binary patch literal 154 zcmV;L0A>Gp0hNwB4#F@H1*v@scc}lL zw%||FO3^N$i%9V=y!@Ff%bxC`m0Y(JQGaVW_lP*0dq(fl-Ro{(__TU)|F< LP!j|IN#_x*?Qaz* literal 0 HcmV?d00001 diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/objects/f2/8eeca3df7614fd4f10c1030f13feb418ef3c6f b/integrations/gitea-repositories-meta/user27/repo49.git/objects/f2/8eeca3df7614fd4f10c1030f13feb418ef3c6f new file mode 100644 index 0000000000000000000000000000000000000000..0699bff833bc1e0a42bfdff36ad7fef2f93505c6 GIT binary patch literal 54 zcmV-60LlM&0V^p=O;s>9XD~D{Ff%bx2y%6F@paY9O=0jg{Jc;t%&e<-V#621E4=(i MIQHoR04~iA?=E#0MF0Q* literal 0 HcmV?d00001 diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/objects/info/packs b/integrations/gitea-repositories-meta/user27/repo49.git/objects/info/packs new file mode 100644 index 0000000000000..8b137891791fe --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/objects/info/packs @@ -0,0 +1 @@ + diff --git a/integrations/gitea-repositories-meta/user27/repo49.git/refs/heads/master b/integrations/gitea-repositories-meta/user27/repo49.git/refs/heads/master new file mode 100644 index 0000000000000..0f13243bfd640 --- /dev/null +++ b/integrations/gitea-repositories-meta/user27/repo49.git/refs/heads/master @@ -0,0 +1 @@ +aacbdfe9e1c4b47f60abe81849045fa4e96f1d75 diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index 3b86dd0f81f91..d146d8b138442 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -674,3 +674,16 @@ num_pulls: 1 is_mirror: false status: 0 + +- + id: 49 + owner_id: 27 + owner_name: user27 + lower_name: repo49 + name: repo49 + is_private: false + num_stars: 0 + num_forks: 0 + num_issues: 0 + is_mirror: false + status: 0 diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index 640fd65bffec1..83baffeb233dd 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -442,7 +442,7 @@ is_admin: false avatar: avatar27 avatar_email: user27@example.com - num_repos: 2 + num_repos: 3 - id: 28 diff --git a/models/repo_list_test.go b/models/repo_list_test.go index ff8b7eb592de6..97047b7ffa400 100644 --- a/models/repo_list_test.go +++ b/models/repo_list_test.go @@ -187,10 +187,10 @@ func TestSearchRepository(t *testing.T) { count: 14}, {name: "AllPublic/PublicRepositoriesOfUserIncludingCollaborative", opts: &SearchRepoOptions{ListOptions: ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, AllPublic: true, Template: util.OptionalBoolFalse}, - count: 25}, + count: 26}, {name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborative", opts: &SearchRepoOptions{ListOptions: ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, Private: true, AllPublic: true, AllLimited: true, Template: util.OptionalBoolFalse}, - count: 30}, + count: 31}, {name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName", opts: &SearchRepoOptions{Keyword: "test", ListOptions: ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, Private: true, AllPublic: true}, count: 15}, @@ -199,7 +199,7 @@ func TestSearchRepository(t *testing.T) { count: 13}, {name: "AllPublic/PublicRepositoriesOfOrganization", opts: &SearchRepoOptions{ListOptions: ListOptions{Page: 1, PageSize: 10}, OwnerID: 17, AllPublic: true, Collaborate: util.OptionalBoolFalse, Template: util.OptionalBoolFalse}, - count: 25}, + count: 26}, {name: "AllTemplates", opts: &SearchRepoOptions{ListOptions: ListOptions{Page: 1, PageSize: 10}, Template: util.OptionalBoolTrue}, count: 2}, diff --git a/routers/repo/repo.go b/routers/repo/repo.go index b0bb608d09b12..78ea55ad35fd6 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -8,24 +8,20 @@ package repo import ( "fmt" "net/url" - "os" - "path" "strings" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/auth" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/context" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/migrations" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/task" "code.gitea.io/gitea/modules/util" + archiver_service "code.gitea.io/gitea/services/archiver" repo_service "code.gitea.io/gitea/services/repository" - - "github.com/unknwon/com" ) const ( @@ -458,80 +454,57 @@ func RedirectDownload(ctx *context.Context) { ctx.Error(404) } -// Download download an archive of a repository -func Download(ctx *context.Context) { - var ( - uri = ctx.Params("*") - refName string - ext string - archivePath string - archiveType git.ArchiveType - ) +// DownloadStatus checks the status of a download, because archiving may take a +// while. It does so by creating an archive request from the archiver service, +// then just examining the completion status. +func DownloadStatus(ctx *context.Context) { + uri := ctx.Params("*") + aReq := archiver_service.DeriveRequestFrom(ctx, uri) - switch { - case strings.HasSuffix(uri, ".zip"): - ext = ".zip" - archivePath = path.Join(ctx.Repo.GitRepo.Path, "archives/zip") - archiveType = git.ZIP - case strings.HasSuffix(uri, ".tar.gz"): - ext = ".tar.gz" - archivePath = path.Join(ctx.Repo.GitRepo.Path, "archives/targz") - archiveType = git.TARGZ - default: - log.Trace("Unknown format: %s", uri) + if aReq == nil { ctx.Error(404) return } - refName = strings.TrimSuffix(uri, ext) - if !com.IsDir(archivePath) { - if err := os.MkdirAll(archivePath, os.ModePerm); err != nil { - ctx.ServerError("Download -> os.MkdirAll(archivePath)", err) - return - } - } + complete := aReq.IsComplete() + ctx.JSON(200, map[string]interface{}{ + "archiving": !complete, + "complete": complete, + }) +} - // Get corresponding commit. - var ( - commit *git.Commit - err error - ) - gitRepo := ctx.Repo.GitRepo - if gitRepo.IsBranchExist(refName) { - commit, err = gitRepo.GetBranchCommit(refName) - if err != nil { - ctx.ServerError("GetBranchCommit", err) - return - } - } else if gitRepo.IsTagExist(refName) { - commit, err = gitRepo.GetTagCommit(refName) - if err != nil { - ctx.ServerError("GetTagCommit", err) - return - } - } else if len(refName) >= 4 && len(refName) <= 40 { - commit, err = gitRepo.GetCommit(refName) - if err != nil { - ctx.NotFound("GetCommit", nil) - return - } +// Download an archive of a repository +func Download(ctx *context.Context) { + uri := ctx.Params("*") + aReq := archiver_service.DeriveRequestFrom(ctx, uri) + + if aReq.IsComplete() { + ctx.ServeFile(aReq.GetArchivePath(), ctx.Repo.Repository.Name+"-"+aReq.GetArchiveName()) } else { - ctx.NotFound("Download", nil) + ctx.Error(404) + } +} + +// InitiateDownload will enqueue an archival request, as needed. It may submit +// a request that's already in-progress, but the archiver service will just +// kind of drop it on the floor if this is the case. +func InitiateDownload(ctx *context.Context) { + uri := ctx.Params("*") + aReq := archiver_service.DeriveRequestFrom(ctx, uri) + + if aReq == nil { return } - archivePath = path.Join(archivePath, base.ShortSha(commit.ID.String())+ext) - if !com.IsFile(archivePath) { - if err := commit.CreateArchive(archivePath, git.CreateArchiveOpts{ - Format: archiveType, - Prefix: setting.Repository.PrefixArchiveFiles, - }); err != nil { - ctx.ServerError("Download -> CreateArchive "+archivePath, err) - return - } + complete := aReq.IsComplete() + if !complete { + archiver_service.ArchiveRepository(aReq) } - ctx.ServeFile(archivePath, ctx.Repo.Repository.Name+"-"+refName+ext) + ctx.JSON(200, map[string]interface{}{ + "archiving": !complete, + "complete": complete, + }) } // Status returns repository's status diff --git a/routers/routes/routes.go b/routers/routes/routes.go index ac0f3f4f14189..a82c071b6d235 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -878,7 +878,11 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/:period", repo.ActivityAuthors) }, context.RepoRef(), repo.MustBeNotEmpty, context.RequireRepoReaderOr(models.UnitTypeCode)) - m.Get("/archive/*", repo.MustBeNotEmpty, reqRepoCodeReader, repo.Download) + m.Group("/archive", func() { + m.Post("/status/*", repo.DownloadStatus) + m.Get("/*", repo.Download) + m.Post("/*", repo.InitiateDownload) + }, repo.MustBeNotEmpty, reqRepoCodeReader) m.Get("/status", reqRepoCodeReader, repo.Status) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go new file mode 100644 index 0000000000000..30b7cc6d0c514 --- /dev/null +++ b/services/archiver/archiver.go @@ -0,0 +1,250 @@ +// Copyright 2020 The Gitea Authors. +// All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package archiver + +import ( + "io" + "io/ioutil" + "os" + "path" + "strings" + "sync" + + "code.gitea.io/gitea/modules/base" + "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" + + "github.com/unknwon/com" +) + +// ArchiveRequest defines the parameters of an archive request, which notably +// includes the specific repository being archived as well as the commit, the +// name by which it was requested, and the kind of archive being requested. +// This is entirely opaque to external entities, though, and mostly used as a +// handle elsewhere. +type ArchiveRequest struct { + uri string + repo *git.Repository + refName string + ext string + archivePath string + archiveType git.ArchiveType + archiveComplete bool + commit *git.Commit +} + +var archiveInProgress []*ArchiveRequest +var archiveMutex sync.Mutex + +// GetArchivePath returns the path from which we can serve this archive. +func (aReq *ArchiveRequest) GetArchivePath() string { + return aReq.archivePath +} + +// GetArchiveName returns the name of the caller, based on the ref used by the +// caller to create this request. +func (aReq *ArchiveRequest) GetArchiveName() string { + return aReq.refName + aReq.ext +} + +// IsComplete returns the completion status of this request. +func (aReq *ArchiveRequest) IsComplete() bool { + return aReq.archiveComplete +} + +// The caller must hold the archiveMutex across calls to getArchiveRequest. +func getArchiveRequest(repo *git.Repository, commit *git.Commit, archiveType git.ArchiveType) *ArchiveRequest { + for _, r := range archiveInProgress { + // Need to be referring to the same repository. + if r.repo.Path == repo.Path && r.commit.ID == commit.ID && r.archiveType == archiveType { + return r + } + } + return nil +} + +// DeriveRequestFrom creates an archival request, based on the URI. The +// resulting ArchiveRequest is suitable for being passed to ArchiveRepository() +// if it's determined that the request still needs to be satisfied. +func DeriveRequestFrom(ctx *context.Context, uri string) *ArchiveRequest { + if ctx.Repo == nil || ctx.Repo.GitRepo == nil { + log.Trace("Repo not initialized") + ctx.Error(404) + return nil + } + r := &ArchiveRequest{ + uri: uri, + repo: ctx.Repo.GitRepo, + } + + switch { + case strings.HasSuffix(uri, ".zip"): + r.ext = ".zip" + r.archivePath = path.Join(r.repo.Path, "archives/zip") + r.archiveType = git.ZIP + case strings.HasSuffix(uri, ".tar.gz"): + r.ext = ".tar.gz" + r.archivePath = path.Join(r.repo.Path, "archives/targz") + r.archiveType = git.TARGZ + default: + log.Trace("Unknown format: %s", uri) + ctx.Error(404) + return nil + } + + r.refName = strings.TrimSuffix(r.uri, r.ext) + if !com.IsDir(r.archivePath) { + if err := os.MkdirAll(r.archivePath, os.ModePerm); err != nil { + ctx.ServerError("Download -> os.MkdirAll(archivePath)", err) + return nil + } + } + + // Get corresponding commit. + var ( + err error + ) + if r.repo.IsBranchExist(r.refName) { + r.commit, err = r.repo.GetBranchCommit(r.refName) + if err != nil { + ctx.ServerError("GetBranchCommit", err) + return nil + } + } else if r.repo.IsTagExist(r.refName) { + r.commit, err = r.repo.GetTagCommit(r.refName) + if err != nil { + ctx.ServerError("GetTagCommit", err) + return nil + } + } else if len(r.refName) >= 4 && len(r.refName) <= 40 { + r.commit, err = r.repo.GetCommit(r.refName) + if err != nil { + ctx.NotFound("GetCommit", nil) + return nil + } + } else { + ctx.NotFound("DeriveRequestFrom", nil) + return nil + } + + archiveMutex.Lock() + defer archiveMutex.Unlock() + if rExisting := getArchiveRequest(r.repo, r.commit, r.archiveType); rExisting != nil { + return rExisting + } + + r.archivePath = path.Join(r.archivePath, base.ShortSha(r.commit.ID.String())+r.ext) + r.archiveComplete = com.IsFile(r.archivePath) + return r +} + +func doArchive(r *ArchiveRequest) { + var ( + err error + tmpArchive *os.File + destArchive *os.File + ) + + // It could have happened that we enqueued two archival requests, due to + // race conditions and difficulties in locking. Do one last check that + // the archive we're referring to doesn't already exist. If it does exist, + // then just mark the request as complete and move on. + if com.IsFile(r.archivePath) { + r.archiveComplete = true + return + } + + // Create a temporary file to use while the archive is being built. We + // will then copy it into place (r.archivePath) once it's fully + // constructed. + tmpArchive, err = ioutil.TempFile("", "archive") + if err != nil { + log.Error("Unable to create a temporary archive file! Error: %v", err) + return + } + defer func() { + tmpArchive.Close() + os.Remove(tmpArchive.Name()) + }() + + if err = r.commit.CreateArchive(tmpArchive.Name(), git.CreateArchiveOpts{ + Format: r.archiveType, + Prefix: setting.Repository.PrefixArchiveFiles, + }); err != nil { + log.Error("Download -> CreateArchive "+tmpArchive.Name(), err) + return + } + + // Now we copy it into place + if destArchive, err = os.Create(r.archivePath); err != nil { + log.Error("Unable to open archive " + r.archivePath) + return + } + _, err = io.Copy(destArchive, tmpArchive) + destArchive.Close() + if err != nil { + log.Error("Unable to write archive " + r.archivePath) + return + } + + r.archiveComplete = true +} + +// ArchiveRepository satisfies the ArchiveRequest being passed in. Processing +// will occur in a separate goroutine, as this phase may take a while to +// complete. If the archive already exists, ArchiveRepository will not do +// anything. +func ArchiveRepository(request *ArchiveRequest) { + if request.archiveComplete { + return + } + go func() { + // We'll take some liberties here, in that the caller may not assume that the + // specific request they submitted is the one getting enqueued. We'll just drop + // it if it turns out we've already enqueued an identical request, as they'll keep + // checking back for the status anyways. + archiveMutex.Lock() + if rExisting := getArchiveRequest(request.repo, request.commit, request.archiveType); rExisting != nil { + archiveMutex.Unlock() + return + } + archiveInProgress = append(archiveInProgress, request) + archiveMutex.Unlock() + + // Drop the mutex while we process the request. This may take a long + // time, and it's not necessary now that we've added the reequest to + // archiveInProgress. + doArchive(request) + + // Purge this request from the list. To do so, we'll just take the + // index at which we ended up at and swap the final element into that + // position, then chop off the now-redundant final element. The slice + // may have change in between these two segments and we may have moved, + // so we search for it here. We could perhaps avoid this search + // entirely if len(archiveInProgress) == 1, but we should verify + // correctness. + archiveMutex.Lock() + defer archiveMutex.Unlock() + idx := -1 + for _idx, req := range archiveInProgress { + if req == request { + idx = _idx + break + } + } + if idx == -1 { + log.Error("ArchiveRepository: Failed to find request for removal.") + return + } + lastidx := len(archiveInProgress) - 1 + if idx != lastidx { + archiveInProgress[idx] = archiveInProgress[lastidx] + } + archiveInProgress = archiveInProgress[:lastidx] + }() +} diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go new file mode 100644 index 0000000000000..98ec4050e4c3b --- /dev/null +++ b/services/archiver/archiver_test.go @@ -0,0 +1,103 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package archiver + +import ( + "path/filepath" + "testing" + "time" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/test" + + "github.com/stretchr/testify/assert" + "github.com/unknwon/com" +) + +func TestMain(m *testing.M) { + models.MainTest(m, filepath.Join("..", "..")) +} + +func TestArchive_Basic(t *testing.T) { + assert.NoError(t, models.PrepareTestDatabase()) + + ctx := test.MockContext(t, "user27/repo49") + firstCommit, secondCommit := "51f84af23134", "aacbdfe9e1c4" + + bogusReq := DeriveRequestFrom(ctx, firstCommit+".zip") + assert.Nil(t, bogusReq) + + test.LoadRepo(t, ctx, 49) + bogusReq = DeriveRequestFrom(ctx, firstCommit+".zip") + assert.Nil(t, bogusReq) + + test.LoadGitRepo(t, ctx) + defer ctx.Repo.GitRepo.Close() + + // Check a series of bogus requests. + // Step 1, valid commit with a bad extension. + bogusReq = DeriveRequestFrom(ctx, firstCommit+".dilbert") + assert.Nil(t, bogusReq) + + // Step 2, missing commit. + bogusReq = DeriveRequestFrom(ctx, "dbffff.zip") + assert.Nil(t, bogusReq) + + // Step 3, doesn't look like branch/tag/commit. + bogusReq = DeriveRequestFrom(ctx, "db.zip") + assert.Nil(t, bogusReq) + + // Now two valid requests, firstCommit with valid extensions. + zipReq := DeriveRequestFrom(ctx, firstCommit+".zip") + assert.NotNil(t, zipReq) + + tgzReq := DeriveRequestFrom(ctx, firstCommit+".tar.gz") + assert.NotNil(t, tgzReq) + + secondReq := DeriveRequestFrom(ctx, secondCommit+".zip") + assert.NotNil(t, secondReq) + + ArchiveRepository(zipReq) + ArchiveRepository(tgzReq) + ArchiveRepository(secondReq) + + // Wait for those requests to complete, time out after 8 seconds. + timeout := time.Now().Add(8 * time.Second) + for { + if zipReq.IsComplete() && tgzReq.IsComplete() && secondReq.IsComplete() { + break + } else if time.Now().After(timeout) { + break + } + } + + assert.True(t, zipReq.IsComplete()) + assert.True(t, tgzReq.IsComplete()) + assert.True(t, secondReq.IsComplete()) + assert.True(t, com.IsExist(zipReq.GetArchivePath())) + assert.True(t, com.IsExist(tgzReq.GetArchivePath())) + assert.True(t, com.IsExist(secondReq.GetArchivePath())) + + // The queue should also be drained, if all requests have completed. + assert.Equal(t, len(archiveInProgress), 0) + + zipReq2 := DeriveRequestFrom(ctx, firstCommit+".zip") + // After completion, zipReq should have dropped out of the queue. Make sure + // we didn't get it handed back to us, but they should otherwise be + // equivalent requests. + assert.Equal(t, zipReq, zipReq2) + assert.False(t, zipReq == zipReq2) + + // Make sure we can submit this follow-up request with no side-effects, to + // the extent that we can. + ArchiveRepository(zipReq2) + assert.Equal(t, zipReq, zipReq2) + assert.Equal(t, len(archiveInProgress), 0) + + // Same commit, different compression formats should have different names. + // Ideally, the extension would match what we originally requested. + assert.NotEqual(t, zipReq.GetArchiveName(), tgzReq.GetArchiveName()) + assert.NotEqual(t, zipReq.GetArchiveName(), secondReq.GetArchiveName()) +} diff --git a/templates/repo/branch/list.tmpl b/templates/repo/branch/list.tmpl index d2eedef49c94e..4928b5d6e7cb9 100644 --- a/templates/repo/branch/list.tmpl +++ b/templates/repo/branch/list.tmpl @@ -27,8 +27,8 @@
- {{svg "octicon-file-zip" 16}} ZIP - {{svg "octicon-file-zip" 16}} TAR.GZ + {{svg "octicon-file-zip" 16}} ZIP + {{svg "octicon-file-zip" 16}} TAR.GZ
@@ -106,8 +106,8 @@
- {{svg "octicon-file-zip" 16}} ZIP - {{svg "octicon-file-zip" 16}} TAR.GZ + {{svg "octicon-file-zip" 16}} ZIP + {{svg "octicon-file-zip" 16}} TAR.GZ
{{end}} diff --git a/templates/repo/home.tmpl b/templates/repo/home.tmpl index 3a0d364f34f5d..55cc68dd1646f 100644 --- a/templates/repo/home.tmpl +++ b/templates/repo/home.tmpl @@ -133,8 +133,8 @@
- {{svg "octicon-file-zip" 16}} ZIP - {{svg "octicon-file-zip" 16}} TAR.GZ + {{svg "octicon-file-zip" 16}} ZIP + {{svg "octicon-file-zip" 16}} TAR.GZ
diff --git a/templates/repo/release/list.tmpl b/templates/repo/release/list.tmpl index d0b160a1c0149..d13fefe8be49b 100644 --- a/templates/repo/release/list.tmpl +++ b/templates/repo/release/list.tmpl @@ -43,8 +43,8 @@
{{if $.Permission.CanRead $.UnitTypeCode}} {{ShortSha .Sha1}} - {{svg "octicon-file-zip" 16}} ZIP - {{svg "octicon-file-zip" 16}} TAR.GZ + {{svg "octicon-file-zip" 16}} ZIP + {{svg "octicon-file-zip" 16}} TAR.GZ {{end}}
{{else}} @@ -80,10 +80,10 @@
    {{if $.Permission.CanRead $.UnitTypeCode}}
  • - {{svg "octicon-file-zip" 16}} {{$.i18n.Tr "repo.release.source_code"}} (ZIP) + {{svg "octicon-file-zip" 16}} {{$.i18n.Tr "repo.release.source_code"}} (ZIP)
  • - {{svg "octicon-file-zip" 16}} {{$.i18n.Tr "repo.release.source_code"}} (TAR.GZ) + {{svg "octicon-file-zip" 16}} {{$.i18n.Tr "repo.release.source_code"}} (TAR.GZ)
    • {{end}} {{if .Attachments}} diff --git a/web_src/js/index.js b/web_src/js/index.js index 639f36fab791e..168d2a60f62e5 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -698,6 +698,86 @@ function initIssueComments() { }); } +function initArchiveStatusChecker($target, url, statusUrl) { + $.ajax({ + url: statusUrl, + type: 'POST', + data: { + _csrf: csrf, + }, + complete(xhr) { + if (xhr.status === 200) { + if (!xhr.responseJSON) { + $target.closest('.dropdown').children('i').removeClass('loading'); + return; + } + + if (xhr.responseJSON.complete) { + // Null out the status URL. We don't need to query status again. + // getArchive() will clear the loading indicator here, as needed. + getArchive($target, url, null); + return; + } + + setTimeout(() => { + initArchiveStatusChecker($target, url, statusUrl); + }, 2000); + } else { + $target.closest('.dropdown').children('i').removeClass('loading'); + } + } + }); +} + +function getArchive($target, url, statusUrl) { + $.ajax({ + url, + type: 'POST', + data: { + _csrf: csrf, + }, + complete(xhr) { + if (xhr.status === 200) { + if (!xhr.responseJSON) { + // XXX Shouldn't happen? + $target.closest('.dropdown').children('i').removeClass('loading'); + return; + } + + if (!xhr.responseJSON.complete && statusUrl !== null) { + $target.closest('.dropdown').children('i').addClass('loading'); + setTimeout(() => { + initArchiveStatusChecker($target, url, statusUrl); + }, 2000); + } else { + // We don't need to continue checking. + $target.closest('.dropdown').children('i').removeClass('loading'); + window.location.href = url; + } + } + } + }); +} + +function initArchiveLinks() { + if ($('.archive-link').length === 0) { + return; + } + + $('.archive-link').on('click', function (event) { + const url = $(this).data('url'); + if (typeof url === 'undefined') { + return; + } + const statusUrl = $(this).data('status'); + if (typeof statusUrl === 'undefined') { + return; + } + + getArchive($(event.target), url, statusUrl); + }); +} + async function initRepository() { if ($('.repository').length === 0) { return; @@ -2565,6 +2645,7 @@ $(document).ready(async () => { initCommentForm(); initInstall(); + initArchiveLinks(); initRepository(); initMigration(); initWikiForm(); From 9397c133537af180f0413e1c6b9bf0733154c8c3 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 16:11:37 -0500 Subject: [PATCH 02/42] Archive links: drop initial delay to three-quarters of a second Some, or perhaps even most, archives will not take all that long to archive. The archive process starts as soon as the download button is initially clicked, so in theory they could be done quite quickly. Drop the initial delay down to three-quarters of a second to make it more responsive in the common case of the archive being quickly created. --- web_src/js/index.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/web_src/js/index.js b/web_src/js/index.js index 168d2a60f62e5..45237fce14d74 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -746,9 +746,11 @@ function getArchive($target, url, statusUrl) { if (!xhr.responseJSON.complete && statusUrl !== null) { $target.closest('.dropdown').children('i').addClass('loading'); + // Wait for only three quarters of a second initially, in case it's + // quickly archived. setTimeout(() => { initArchiveStatusChecker($target, url, statusUrl); - }, 2000); + }, 750); } else { // We don't need to continue checking. $target.closest('.dropdown').children('i').removeClass('loading'); From 6785d390f7e8ffbbb836ae1464c8fc2dbd4c9efd Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 16:36:04 -0500 Subject: [PATCH 03/42] archiver: restructure a little bit to facilitate testing This introduces two sync.Cond pointers to the archiver package. If they're non-nil when we go to process a request, we'll wait until signalled (at all) to proceed. The tests will then create the sync.Cond so that it can signal at-will and sanity-check the state of the queue at different phases. The author believes that nil-checking these two sync.Cond pointers on every archive processing will introduce minimal overhead with no impact on maintainability. --- services/archiver/archiver.go | 21 ++++++++ services/archiver/archiver_test.go | 86 +++++++++++++++++++++++++++--- 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index 30b7cc6d0c514..ebc060e943a34 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -41,6 +41,12 @@ type ArchiveRequest struct { var archiveInProgress []*ArchiveRequest var archiveMutex sync.Mutex +// These facilitate testing, by allowing the unit tests to control (to some extent) +// the goroutine used for processing the queue. +var archiveQueueMutex *sync.Mutex +var archiveQueueStartCond *sync.Cond +var archiveQueueReleaseCond *sync.Cond + // GetArchivePath returns the path from which we can serve this archive. func (aReq *ArchiveRequest) GetArchivePath() string { return aReq.archivePath @@ -216,11 +222,26 @@ func ArchiveRepository(request *ArchiveRequest) { archiveInProgress = append(archiveInProgress, request) archiveMutex.Unlock() + // Wait to start, if we have the Cond for it. This is currently only + // useful for testing, so that the start and release of queued entries + // can be controlled to examine the queue. + if archiveQueueStartCond != nil { + archiveQueueMutex.Lock() + archiveQueueStartCond.Wait() + archiveQueueMutex.Unlock() + } + // Drop the mutex while we process the request. This may take a long // time, and it's not necessary now that we've added the reequest to // archiveInProgress. doArchive(request) + if archiveQueueReleaseCond != nil { + archiveQueueMutex.Lock() + archiveQueueReleaseCond.Wait() + archiveQueueMutex.Unlock() + } + // Purge this request from the list. To do so, we'll just take the // index at which we ended up at and swap the final element into that // position, then chop off the now-redundant final element. The slice diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 98ec4050e4c3b..4b6574bfd87bf 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -6,6 +6,7 @@ package archiver import ( "path/filepath" + "sync" "testing" "time" @@ -16,8 +17,46 @@ import ( "github.com/unknwon/com" ) +var queueMutex sync.Mutex + func TestMain(m *testing.M) { models.MainTest(m, filepath.Join("..", "..")) + + archiveQueueMutex = &queueMutex + archiveQueueStartCond = sync.NewCond(&queueMutex) + archiveQueueReleaseCond = sync.NewCond(&queueMutex) +} + +func allComplete(inFlight []*ArchiveRequest) bool { + for _, req := range inFlight { + if !req.IsComplete() { + return false + } + } + + return true +} + +func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { + var nowQueued, numQueued int + + numQueued = len(archiveInProgress) + + // Release one, then wait up to 3 seconds for it to complete. + archiveQueueReleaseCond.Signal() + timeout := time.Now().Add(3 * time.Second) + for { + nowQueued = len(archiveInProgress) + if nowQueued != numQueued || time.Now().After(timeout) { + break + } + } + + // Make sure we didn't just timeout. + assert.NotEqual(t, nowQueued, numQueued) + + // Also make sure that we released only one. + assert.Equal(t, nowQueued, numQueued + 1) } func TestArchive_Basic(t *testing.T) { @@ -59,14 +98,31 @@ func TestArchive_Basic(t *testing.T) { secondReq := DeriveRequestFrom(ctx, secondCommit+".zip") assert.NotNil(t, secondReq) + inFlight := make([]*ArchiveRequest, 3) + inFlight[0] = zipReq + inFlight[1] = tgzReq + inFlight[2] = secondReq + ArchiveRepository(zipReq) + assert.Equal(t, len(archiveInProgress), 1) ArchiveRepository(tgzReq) + assert.Equal(t, len(archiveInProgress), 2) ArchiveRepository(secondReq) + assert.Equal(t, len(archiveInProgress), 3) + + // Make sure sending an unprocessed request through doesn't affect the queue + // count. + ArchiveRepository(zipReq) + assert.Equal(t, len(archiveInProgress), 3) + + // Release them all, they'll then stall at the archiveQueueReleaseCond while + // we examine the queue state. + archiveQueueStartCond.Broadcast() - // Wait for those requests to complete, time out after 8 seconds. + // 8 second timeout for them all to complete. timeout := time.Now().Add(8 * time.Second) for { - if zipReq.IsComplete() && tgzReq.IsComplete() && secondReq.IsComplete() { + if allComplete(inFlight) { break } else if time.Now().After(timeout) { break @@ -80,8 +136,9 @@ func TestArchive_Basic(t *testing.T) { assert.True(t, com.IsExist(tgzReq.GetArchivePath())) assert.True(t, com.IsExist(secondReq.GetArchivePath())) - // The queue should also be drained, if all requests have completed. - assert.Equal(t, len(archiveInProgress), 0) + // Queues should not have drained yet, because we haven't released them. + // Do so now. + assert.Equal(t, len(archiveInProgress), 3) zipReq2 := DeriveRequestFrom(ctx, firstCommit+".zip") // After completion, zipReq should have dropped out of the queue. Make sure @@ -90,10 +147,25 @@ func TestArchive_Basic(t *testing.T) { assert.Equal(t, zipReq, zipReq2) assert.False(t, zipReq == zipReq2) - // Make sure we can submit this follow-up request with no side-effects, to - // the extent that we can. + // We still have the other three stalled at completion, waiting to remove + // from archiveInProgress. Try to submit this new one before its + // predecessor has cleared out of the queue. ArchiveRepository(zipReq2) - assert.Equal(t, zipReq, zipReq2) + + // Make sure we didn't enqueue anything from this new one, and that the + // queue hasn't changed. + assert.Equal(t, len(archiveInProgress), 3) + + for _, req := range archiveInProgress { + assert.False(t, req == zipReq2) + } + + // Make sure the queue drains properly + releaseOneEntry(t, inFlight) + assert.Equal(t, len(archiveInProgress), 2) + releaseOneEntry(t, inFlight) + assert.Equal(t, len(archiveInProgress), 1) + releaseOneEntry(t, inFlight) assert.Equal(t, len(archiveInProgress), 0) // Same commit, different compression formats should have different names. From 5434d6eaf88fba15fc2143d01cb694814851da37 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 16:52:40 -0500 Subject: [PATCH 04/42] gofmt nit: no space around binary + operator --- services/archiver/archiver_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 4b6574bfd87bf..71a4fb905b6f9 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -56,7 +56,7 @@ func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { assert.NotEqual(t, nowQueued, numQueued) // Also make sure that we released only one. - assert.Equal(t, nowQueued, numQueued + 1) + assert.Equal(t, nowQueued, numQueued+1) } func TestArchive_Basic(t *testing.T) { From 3b8eac493b9aee8b169bd1f81f70d5a40cee1c0a Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 16:56:39 -0500 Subject: [PATCH 05/42] services: archiver: appease golangci-lint, lock queueMutex Locking/unlocking the queueMutex is allowed, but not required, for Cond.Signal() and Cond.Broadcast(). The magic at play here is just a little too much for golangci-lint, as we take the address of queueMutex and this is mostly used in archiver.go; the variable still gets flagged as unused. --- services/archiver/archiver_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 71a4fb905b6f9..bfeeb24f1e786 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -43,7 +43,9 @@ func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { numQueued = len(archiveInProgress) // Release one, then wait up to 3 seconds for it to complete. + queueMutex.Lock() archiveQueueReleaseCond.Signal() + queueMutex.Unlock() timeout := time.Now().Add(3 * time.Second) for { nowQueued = len(archiveInProgress) @@ -117,7 +119,9 @@ func TestArchive_Basic(t *testing.T) { // Release them all, they'll then stall at the archiveQueueReleaseCond while // we examine the queue state. + queueMutex.Lock() archiveQueueStartCond.Broadcast() + queueMutex.Unlock() // 8 second timeout for them all to complete. timeout := time.Now().Add(8 * time.Second) From 59786fe125fe4cd3efd277657f2996d228675ab8 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 17:30:21 -0500 Subject: [PATCH 06/42] archiver: tests: fix several timing nits Once we've signaled a cond var, it may take some small amount of time for the goroutines released to hit the spot we're wanting them to be at. Give them an appropriate amount of time. --- services/archiver/archiver_test.go | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index bfeeb24f1e786..cfbe646db9090 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -37,6 +37,21 @@ func allComplete(inFlight []*ArchiveRequest) bool { return true } +func waitForCount(t *testing.T, num int) { + var numQueued int + + // Wait for 3 seconds to hit the queue. + timeout := time.Now().Add(3 * time.Second) + for { + numQueued = len(archiveInProgress) + if numQueued == num || time.Now().After(timeout) { + break + } + } + + assert.Equal(t, num, len(archiveInProgress)) +} + func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { var nowQueued, numQueued int @@ -58,7 +73,7 @@ func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { assert.NotEqual(t, nowQueued, numQueued) // Also make sure that we released only one. - assert.Equal(t, nowQueued, numQueued+1) + assert.Equal(t, nowQueued, numQueued-1) } func TestArchive_Basic(t *testing.T) { @@ -106,16 +121,20 @@ func TestArchive_Basic(t *testing.T) { inFlight[2] = secondReq ArchiveRepository(zipReq) - assert.Equal(t, len(archiveInProgress), 1) + waitForCount(t, 1) ArchiveRepository(tgzReq) - assert.Equal(t, len(archiveInProgress), 2) + waitForCount(t, 2) ArchiveRepository(secondReq) - assert.Equal(t, len(archiveInProgress), 3) + waitForCount(t, 3) // Make sure sending an unprocessed request through doesn't affect the queue // count. ArchiveRepository(zipReq) - assert.Equal(t, len(archiveInProgress), 3) + + // Sleep two seconds to make sure the queue doesn't change. + two_seconds, _ := time.ParseDuration("2s") + time.Sleep(two_seconds) + assert.Equal(t, 3, len(archiveInProgress)) // Release them all, they'll then stall at the archiveQueueReleaseCond while // we examine the queue state. From 759b4d1eec9bc6cfe11bce679cec72a95c1c8317 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 17:35:17 -0500 Subject: [PATCH 07/42] archiver: tests: no underscore in var name, ungh --- services/archiver/archiver_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index cfbe646db9090..4c0ee4407d5a9 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -132,8 +132,8 @@ func TestArchive_Basic(t *testing.T) { ArchiveRepository(zipReq) // Sleep two seconds to make sure the queue doesn't change. - two_seconds, _ := time.ParseDuration("2s") - time.Sleep(two_seconds) + twoSeconds, _ := time.ParseDuration("2s") + time.Sleep(twoSeconds) assert.Equal(t, 3, len(archiveInProgress)) // Release them all, they'll then stall at the archiveQueueReleaseCond while From be77f9ed593bb6aec0a35b617b059f920e6b4481 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 17:49:26 -0500 Subject: [PATCH 08/42] archiver: tests: Test* is run in a separate context than TestMain We must setup the mutex/cond variables at the beginning of any test that's going to use it, or else these will be nil when the test is actually ran. --- services/archiver/archiver_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 4c0ee4407d5a9..63ef36fb0fdf9 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -21,10 +21,6 @@ var queueMutex sync.Mutex func TestMain(m *testing.M) { models.MainTest(m, filepath.Join("..", "..")) - - archiveQueueMutex = &queueMutex - archiveQueueStartCond = sync.NewCond(&queueMutex) - archiveQueueReleaseCond = sync.NewCond(&queueMutex) } func allComplete(inFlight []*ArchiveRequest) bool { @@ -79,6 +75,15 @@ func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { func TestArchive_Basic(t *testing.T) { assert.NoError(t, models.PrepareTestDatabase()) + archiveQueueMutex = &queueMutex + archiveQueueStartCond = sync.NewCond(&queueMutex) + archiveQueueReleaseCond = sync.NewCond(&queueMutex) + defer func() { + archiveQueueMutex = nil + archiveQueueStartCond = nil + archiveQueueReleaseCond = nil + }() + ctx := test.MockContext(t, "user27/repo49") firstCommit, secondCommit := "51f84af23134", "aacbdfe9e1c4" From 67d491ba7c2a7a06fce00a017a3a561b9823f3c4 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 18:04:20 -0500 Subject: [PATCH 09/42] archiver: tests: hopefully final tweak Things got shuffled around such that we carefully build up and release requests from the queue, so we can validate the state of the queue at each step. Fix some assertions that no longer hold true as fallout. --- services/archiver/archiver_test.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 63ef36fb0fdf9..ee5904ef0b0e2 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -169,25 +169,19 @@ func TestArchive_Basic(t *testing.T) { assert.Equal(t, len(archiveInProgress), 3) zipReq2 := DeriveRequestFrom(ctx, firstCommit+".zip") - // After completion, zipReq should have dropped out of the queue. Make sure - // we didn't get it handed back to us, but they should otherwise be - // equivalent requests. + // This zipReq should match what's sitting in the queue, as we haven't + // let it release yet. From the consumer's point of view, this looks like + // a long-running archive task. assert.Equal(t, zipReq, zipReq2) - assert.False(t, zipReq == zipReq2) // We still have the other three stalled at completion, waiting to remove // from archiveInProgress. Try to submit this new one before its // predecessor has cleared out of the queue. ArchiveRepository(zipReq2) - // Make sure we didn't enqueue anything from this new one, and that the - // queue hasn't changed. + // Make sure the queue hasn't grown any. assert.Equal(t, len(archiveInProgress), 3) - for _, req := range archiveInProgress { - assert.False(t, req == zipReq2) - } - // Make sure the queue drains properly releaseOneEntry(t, inFlight) assert.Equal(t, len(archiveInProgress), 2) @@ -196,6 +190,13 @@ func TestArchive_Basic(t *testing.T) { releaseOneEntry(t, inFlight) assert.Equal(t, len(archiveInProgress), 0) + zipReq2 = DeriveRequestFrom(ctx, firstCommit+".zip") + // Now, we're guaranteed to have released the original zipReq from the queue. + // Ensure that we don't get handed back the released entry somehow, but they + // should remain functionally equivalent in all fields. + assert.Equal(t, zipReq, zipReq2) + assert.False(t, zipReq == zipReq2) + // Same commit, different compression formats should have different names. // Ideally, the extension would match what we originally requested. assert.NotEqual(t, zipReq.GetArchiveName(), tgzReq.GetArchiveName()) From c134aef340376f9a51fe1cf07f185ad407dc0dd4 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 18:56:13 -0500 Subject: [PATCH 10/42] repo: Download: restore some semblance of previous behavior When archival was made async, the GET endpoint was only useful if a previous POST had initiated the download. This commit restores the previous behavior, to an extent; we'll now submit the archive request there and return a "202 Accepted" to indicate that it's processing if we didn't manage to complete the request within ~2 seconds of submission. This lets a client directly GET the archive, and gives them some indication that they may attempt to GET it again at a later time. --- routers/repo/repo.go | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/routers/repo/repo.go b/routers/repo/repo.go index 78ea55ad35fd6..95d4701be9958 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -9,6 +9,7 @@ import ( "fmt" "net/url" "strings" + "time" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/auth" @@ -478,10 +479,24 @@ func Download(ctx *context.Context) { uri := ctx.Params("*") aReq := archiver_service.DeriveRequestFrom(ctx, uri) + downloadName := ctx.Repo.Repository.Name + "-" + aReq.GetArchiveName() if aReq.IsComplete() { - ctx.ServeFile(aReq.GetArchivePath(), ctx.Repo.Repository.Name+"-"+aReq.GetArchiveName()) + ctx.ServeFile(aReq.GetArchivePath(), downloadName) } else { - ctx.Error(404) + // We'll wait up to two seconds for the request to be satisfied, before we just return + // a 200 Accepted to indicate that we're processing. + archiver_service.ArchiveRepository(aReq) + timeout := time.Now().Add(2 * time.Second) + for { + if aReq.IsComplete() || time.Now().After(timeout) { + break + } + } + if aReq.IsComplete() { + ctx.ServeFile(aReq.GetArchivePath(), downloadName) + } else { + ctx.Error(202, "Request accepted, processing archive.") + } } } @@ -499,6 +514,18 @@ func InitiateDownload(ctx *context.Context) { complete := aReq.IsComplete() if !complete { archiver_service.ArchiveRepository(aReq) + // As with the standard Download, we'll wait up to two seconds for the request + // to be completed. The difference is that we'll never download the file from a POST + // request, only indicate the current status. If we did manage to complete the request + // in this timeframe, the download will proceed with no further overhead. + timeout := time.Now().Add(2 * time.Second) + for { + if aReq.IsComplete() || time.Now().After(timeout) { + break + } + } + + complete = aReq.IsComplete() } ctx.JSON(200, map[string]interface{}{ From 6b25f4385337fdb9dbf08cbfc28a52e6fb9b3742 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 19:13:57 -0500 Subject: [PATCH 11/42] archiver: tests: simplify a bit further We don't need to risk failure and use time.ParseDuration to get 2 * time.Second. else if isn't really necessary if the conditions are simple enough and lead to the same result. --- services/archiver/archiver_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index ee5904ef0b0e2..5b38cb55e9ecb 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -137,8 +137,7 @@ func TestArchive_Basic(t *testing.T) { ArchiveRepository(zipReq) // Sleep two seconds to make sure the queue doesn't change. - twoSeconds, _ := time.ParseDuration("2s") - time.Sleep(twoSeconds) + time.Sleep(2 * time.Second) assert.Equal(t, 3, len(archiveInProgress)) // Release them all, they'll then stall at the archiveQueueReleaseCond while @@ -150,9 +149,7 @@ func TestArchive_Basic(t *testing.T) { // 8 second timeout for them all to complete. timeout := time.Now().Add(8 * time.Second) for { - if allComplete(inFlight) { - break - } else if time.Now().After(timeout) { + if allComplete(inFlight) || time.Now().After(timeout) { break } } From eb15781d983325301d04720f4f70cd5cd0151cbb Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 19:31:14 -0500 Subject: [PATCH 12/42] archiver: tests: resolve potential source of flakiness Increase all timeouts to 10 seconds; these aren't hard-coded sleeps, so there's no guarantee we'll actually take that long. If we need longer to not have a false-positive, then so be it. While here, various assert.{Not,}Equal arguments are flipped around so that the wording in error output reflects reality, where the expected argument is second and actual third. --- services/archiver/archiver_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 5b38cb55e9ecb..61f9e72958012 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -36,8 +36,8 @@ func allComplete(inFlight []*ArchiveRequest) bool { func waitForCount(t *testing.T, num int) { var numQueued int - // Wait for 3 seconds to hit the queue. - timeout := time.Now().Add(3 * time.Second) + // Wait for up to 10 seconds for the queue to be impacted. + timeout := time.Now().Add(10 * time.Second) for { numQueued = len(archiveInProgress) if numQueued == num || time.Now().After(timeout) { @@ -53,11 +53,11 @@ func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { numQueued = len(archiveInProgress) - // Release one, then wait up to 3 seconds for it to complete. + // Release one, then wait up to 10 seconds for it to complete. queueMutex.Lock() archiveQueueReleaseCond.Signal() queueMutex.Unlock() - timeout := time.Now().Add(3 * time.Second) + timeout := time.Now().Add(10 * time.Second) for { nowQueued = len(archiveInProgress) if nowQueued != numQueued || time.Now().After(timeout) { @@ -66,10 +66,10 @@ func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { } // Make sure we didn't just timeout. - assert.NotEqual(t, nowQueued, numQueued) + assert.NotEqual(t, numQueued, nowQueued) // Also make sure that we released only one. - assert.Equal(t, nowQueued, numQueued-1) + assert.Equal(t, numQueued-1, nowQueued) } func TestArchive_Basic(t *testing.T) { @@ -146,8 +146,8 @@ func TestArchive_Basic(t *testing.T) { archiveQueueStartCond.Broadcast() queueMutex.Unlock() - // 8 second timeout for them all to complete. - timeout := time.Now().Add(8 * time.Second) + // 10 second timeout for them all to complete. + timeout := time.Now().Add(10 * time.Second) for { if allComplete(inFlight) || time.Now().After(timeout) { break @@ -177,15 +177,15 @@ func TestArchive_Basic(t *testing.T) { ArchiveRepository(zipReq2) // Make sure the queue hasn't grown any. - assert.Equal(t, len(archiveInProgress), 3) + assert.Equal(t, 3, len(archiveInProgress)) // Make sure the queue drains properly releaseOneEntry(t, inFlight) - assert.Equal(t, len(archiveInProgress), 2) + assert.Equal(t, 2, len(archiveInProgress)) releaseOneEntry(t, inFlight) - assert.Equal(t, len(archiveInProgress), 1) + assert.Equal(t, 1, len(archiveInProgress)) releaseOneEntry(t, inFlight) - assert.Equal(t, len(archiveInProgress), 0) + assert.Equal(t, 0, len(archiveInProgress)) zipReq2 = DeriveRequestFrom(ctx, firstCommit+".zip") // Now, we're guaranteed to have released the original zipReq from the queue. From d134c3f3cee164cf641cb1b372afdb20496dbcf0 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 22:36:07 -0500 Subject: [PATCH 13/42] archiver: setup infrastructure for notifying consumers of completion This API will *not* allow consumers to subscribe to specific requests being completed, just *any* request being completed. The caller is responsible for determining if their request is satisfied and waiting again if needed. --- routers/init.go | 2 ++ services/archiver/archiver.go | 68 +++++++++++++++++++++++++++-------- 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/routers/init.go b/routers/init.go index 724bf84c10fef..9e97b5dade5bb 100644 --- a/routers/init.go +++ b/routers/init.go @@ -28,6 +28,7 @@ import ( "code.gitea.io/gitea/modules/ssh" "code.gitea.io/gitea/modules/task" "code.gitea.io/gitea/modules/webhook" + "code.gitea.io/gitea/services/archiver" "code.gitea.io/gitea/services/mailer" mirror_service "code.gitea.io/gitea/services/mirror" pull_service "code.gitea.io/gitea/services/pull" @@ -50,6 +51,7 @@ func checkRunMode() { // NewServices init new services func NewServices() { setting.NewServices() + archiver.NewContext() mailer.NewContext() _ = cache.NewContext() notification.NewContext() diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index ebc060e943a34..cbf70a8d300ca 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -40,6 +40,7 @@ type ArchiveRequest struct { var archiveInProgress []*ArchiveRequest var archiveMutex sync.Mutex +var archiveCond *sync.Cond // These facilitate testing, by allowing the unit tests to control (to some extent) // the goroutine used for processing the queue. @@ -198,30 +199,33 @@ func doArchive(r *ArchiveRequest) { return } + // Block any attempt to finalize creating a new request if we're marking r.archiveComplete = true } // ArchiveRepository satisfies the ArchiveRequest being passed in. Processing // will occur in a separate goroutine, as this phase may take a while to // complete. If the archive already exists, ArchiveRepository will not do -// anything. -func ArchiveRepository(request *ArchiveRequest) { - if request.archiveComplete { - return +// anything. In all cases, the caller should be examining the *ArchiveRequest +// being returned for completion, as it may be different than the one they passed +// in. +func ArchiveRepository(request *ArchiveRequest) *ArchiveRequest { + // We'll return the request that's already been enqueued if it has been + // enqueued, or we'll immediately enqueue it if it has not been enqueued + // and it is not marked complete. + archiveMutex.Lock() + if rExisting := getArchiveRequest(request.repo, request.commit, request.archiveType); rExisting != nil { + archiveMutex.Unlock() + return rExisting } - go func() { - // We'll take some liberties here, in that the caller may not assume that the - // specific request they submitted is the one getting enqueued. We'll just drop - // it if it turns out we've already enqueued an identical request, as they'll keep - // checking back for the status anyways. - archiveMutex.Lock() - if rExisting := getArchiveRequest(request.repo, request.commit, request.archiveType); rExisting != nil { - archiveMutex.Unlock() - return - } - archiveInProgress = append(archiveInProgress, request) + if request.archiveComplete { archiveMutex.Unlock() + return request + } + archiveInProgress = append(archiveInProgress, request) + archiveMutex.Unlock() + go func() { // Wait to start, if we have the Cond for it. This is currently only // useful for testing, so that the start and release of queued entries // can be controlled to examine the queue. @@ -251,6 +255,11 @@ func ArchiveRepository(request *ArchiveRequest) { // correctness. archiveMutex.Lock() defer archiveMutex.Unlock() + // Wake up all other goroutines that may be waiting on a request to + // complete. They should all wake up, see if that particular request + // is complete, then return to waiting if it is not. + archiveCond.Broadcast() + idx := -1 for _idx, req := range archiveInProgress { if req == request { @@ -268,4 +277,33 @@ func ArchiveRepository(request *ArchiveRequest) { } archiveInProgress = archiveInProgress[:lastidx] }() + + return request +} + +// LockQueue will obtain the archiveMutex for the caller. This allows the +// underlying locking mechanism to remain opaque. +func LockQueue() { + archiveMutex.Lock() +} + +// UnlockQueue will release the archiveMutex for the caller, again allowing the +// underlying locking mechanism to remain opaque. +func UnlockQueue() { + archiveMutex.Unlock() +} + +// WaitForCompletion should be called with the queue locked (LockQueue), and will +// return with the queue lock held when a single archive request has finished. +// There is currently no API for getting notified of a particular request being +// completed. +func WaitForCompletion() { + archiveCond.Wait() +} + +// NewContext will initialize local state, e.g. primitives needed to be able to +// synchronize with the lock queue and allow callers to wait for an archive to +// finish. +func NewContext() { + archiveCond = sync.NewCond(&archiveMutex) } From ee6f508799e7c0e23eb0534c4f139b9e0f3d6760 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 5 May 2020 22:37:12 -0500 Subject: [PATCH 14/42] repo: archive: make GET endpoint synchronous again If the request isn't complete, this endpoint will now submit the request and wait for completion using the new API. This may still be susceptible to timeouts for larger repos, but other endpoints now exist that the web interface will use to negotiate its way through larger archive processes. --- routers/repo/repo.go | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/routers/repo/repo.go b/routers/repo/repo.go index 95d4701be9958..3f18f36721450 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -480,23 +480,19 @@ func Download(ctx *context.Context) { aReq := archiver_service.DeriveRequestFrom(ctx, uri) downloadName := ctx.Repo.Repository.Name + "-" + aReq.GetArchiveName() + if !aReq.IsComplete() { + aReq = archiver_service.ArchiveRepository(aReq) + archiver_service.LockQueue() + for !aReq.IsComplete() { + archiver_service.WaitForCompletion() + } + archiver_service.UnlockQueue() + } + if aReq.IsComplete() { ctx.ServeFile(aReq.GetArchivePath(), downloadName) } else { - // We'll wait up to two seconds for the request to be satisfied, before we just return - // a 200 Accepted to indicate that we're processing. - archiver_service.ArchiveRepository(aReq) - timeout := time.Now().Add(2 * time.Second) - for { - if aReq.IsComplete() || time.Now().After(timeout) { - break - } - } - if aReq.IsComplete() { - ctx.ServeFile(aReq.GetArchivePath(), downloadName) - } else { - ctx.Error(202, "Request accepted, processing archive.") - } + ctx.Error(404) } } @@ -513,7 +509,7 @@ func InitiateDownload(ctx *context.Context) { complete := aReq.IsComplete() if !complete { - archiver_service.ArchiveRepository(aReq) + aReq = archiver_service.ArchiveRepository(aReq) // As with the standard Download, we'll wait up to two seconds for the request // to be completed. The difference is that we'll never download the file from a POST // request, only indicate the current status. If we did manage to complete the request From 75261f56bc05d1fa8ff7e81dcbc0ccd93fdc9d50 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Thu, 7 May 2020 18:38:47 -0500 Subject: [PATCH 15/42] archiver: tests: amend test to include WaitForCompletion() This is a trivial one, so go ahead and include it. --- services/archiver/archiver_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 61f9e72958012..54ea6f1f1fbe7 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -184,7 +184,15 @@ func TestArchive_Basic(t *testing.T) { assert.Equal(t, 2, len(archiveInProgress)) releaseOneEntry(t, inFlight) assert.Equal(t, 1, len(archiveInProgress)) + + // Test waiting for completion on one, which should be relatively + // straightforward. We'll hold the queue-lock and release an entry. It will + // wait to acquire the queue lock, which we'll drop when we + // WaitForCompletion(), to be woken up later. + LockQueue() releaseOneEntry(t, inFlight) + WaitForCompletion() + UnlockQueue() assert.Equal(t, 0, len(archiveInProgress)) zipReq2 = DeriveRequestFrom(ctx, firstCommit+".zip") From 709c35685eaaf261ebbb7d3420e3376a4ee8e7f2 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Thu, 7 May 2020 19:02:50 -0500 Subject: [PATCH 16/42] archiver: tests: fix test by calling NewContext() The mutex is otherwise uninitialized, so we need to ensure that we're actually initializing it if we plan to test it. --- services/archiver/archiver_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 54ea6f1f1fbe7..77f9199788e96 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -75,6 +75,9 @@ func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { func TestArchive_Basic(t *testing.T) { assert.NoError(t, models.PrepareTestDatabase()) + // Create a new context here, because we may want to use locks or need other + // initial state here. + NewContext() archiveQueueMutex = &queueMutex archiveQueueStartCond = sync.NewCond(&queueMutex) archiveQueueReleaseCond = sync.NewCond(&queueMutex) From 9fc8bedb5667d24d3a3c7843dc28a229efffb1e6 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Thu, 7 May 2020 19:23:52 -0500 Subject: [PATCH 17/42] archiver: tests: integrate new WaitForCompletion a little better We can use this to wait for archives to come in, rather than spinning and hoping with a timeout. --- services/archiver/archiver_test.go | 34 ++++++++++-------------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 77f9199788e96..e2bcc3da9e9b4 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -49,27 +49,23 @@ func waitForCount(t *testing.T, num int) { } func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { - var nowQueued, numQueued int + var numQueued int numQueued = len(archiveInProgress) - // Release one, then wait up to 10 seconds for it to complete. - queueMutex.Lock() + // Release one, then WaitForCompletion. We'll get signalled when ready. + // This works out to be quick and race-free, as we'll get signalled when the + // archival goroutine proceeds to dequeue the now-complete archive but we + // can't pick up the queue lock again until it's done removing it from + // archiveInProgress. We'll remain waiting on the queue lock in + // WaitForCompletion() until we can safely acquire the lock. + LockQueue() archiveQueueReleaseCond.Signal() - queueMutex.Unlock() - timeout := time.Now().Add(10 * time.Second) - for { - nowQueued = len(archiveInProgress) - if nowQueued != numQueued || time.Now().After(timeout) { - break - } - } - - // Make sure we didn't just timeout. - assert.NotEqual(t, numQueued, nowQueued) + WaitForCompletion() + UnlockQueue() // Also make sure that we released only one. - assert.Equal(t, numQueued-1, nowQueued) + assert.Equal(t, numQueued-1, len(archiveInProgress)) } func TestArchive_Basic(t *testing.T) { @@ -187,15 +183,7 @@ func TestArchive_Basic(t *testing.T) { assert.Equal(t, 2, len(archiveInProgress)) releaseOneEntry(t, inFlight) assert.Equal(t, 1, len(archiveInProgress)) - - // Test waiting for completion on one, which should be relatively - // straightforward. We'll hold the queue-lock and release an entry. It will - // wait to acquire the queue lock, which we'll drop when we - // WaitForCompletion(), to be woken up later. - LockQueue() releaseOneEntry(t, inFlight) - WaitForCompletion() - UnlockQueue() assert.Equal(t, 0, len(archiveInProgress)) zipReq2 = DeriveRequestFrom(ctx, firstCommit+".zip") From bcc52140238e16680f2e05e448e9be51372afdf5 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Thu, 7 May 2020 19:34:03 -0500 Subject: [PATCH 18/42] archiver: tests: combine numQueued declaration with next-instruction assignment --- services/archiver/archiver_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index e2bcc3da9e9b4..5ff8c8ecf81dd 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -49,9 +49,7 @@ func waitForCount(t *testing.T, num int) { } func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { - var numQueued int - - numQueued = len(archiveInProgress) + numQueued := len(archiveInProgress) // Release one, then WaitForCompletion. We'll get signalled when ready. // This works out to be quick and race-free, as we'll get signalled when the From 27155abda73718652c1333a2fdc96c28d97dd086 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Thu, 7 May 2020 23:08:36 -0500 Subject: [PATCH 19/42] routers: repo: reap unused archiving flag from DownloadStatus() This had some planned usage before, indicating whether this request initiated the archival process or not. After several rounds of refactoring, this use was deemed not necessary for much of anything and got boiled down to !complete in all cases. --- routers/repo/repo.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/routers/repo/repo.go b/routers/repo/repo.go index 3f18f36721450..e27f37f68ccee 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -469,7 +469,6 @@ func DownloadStatus(ctx *context.Context) { complete := aReq.IsComplete() ctx.JSON(200, map[string]interface{}{ - "archiving": !complete, "complete": complete, }) } @@ -525,7 +524,6 @@ func InitiateDownload(ctx *context.Context) { } ctx.JSON(200, map[string]interface{}{ - "archiving": !complete, "complete": complete, }) } From 9e97b7c8513edffaaea26cf7603d58f22d4b6144 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Thu, 7 May 2020 23:56:52 -0500 Subject: [PATCH 20/42] services: archiver: restructure to use a channel We now offer two forms of waiting for a request: - WaitForCompletion: wait for completion with no timeout - TimedWaitForCompletion: wait for completion with timeout In both cases, we wait for the given request's cchan to close; in the latter case, we do so with the caller-provided timeout. This completely removes the need for busy-wait loops in Download/InitiateDownload, as it's fairly clean to wait on a channel with timeout. --- routers/init.go | 2 -- routers/repo/repo.go | 24 +++---------- services/archiver/archiver.go | 64 +++++++++++++++++------------------ 3 files changed, 37 insertions(+), 53 deletions(-) diff --git a/routers/init.go b/routers/init.go index 9e97b5dade5bb..724bf84c10fef 100644 --- a/routers/init.go +++ b/routers/init.go @@ -28,7 +28,6 @@ import ( "code.gitea.io/gitea/modules/ssh" "code.gitea.io/gitea/modules/task" "code.gitea.io/gitea/modules/webhook" - "code.gitea.io/gitea/services/archiver" "code.gitea.io/gitea/services/mailer" mirror_service "code.gitea.io/gitea/services/mirror" pull_service "code.gitea.io/gitea/services/pull" @@ -51,7 +50,6 @@ func checkRunMode() { // NewServices init new services func NewServices() { setting.NewServices() - archiver.NewContext() mailer.NewContext() _ = cache.NewContext() notification.NewContext() diff --git a/routers/repo/repo.go b/routers/repo/repo.go index e27f37f68ccee..6910bf3ceed33 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -479,16 +479,13 @@ func Download(ctx *context.Context) { aReq := archiver_service.DeriveRequestFrom(ctx, uri) downloadName := ctx.Repo.Repository.Name + "-" + aReq.GetArchiveName() - if !aReq.IsComplete() { + complete := aReq.IsComplete() + if !complete { aReq = archiver_service.ArchiveRepository(aReq) - archiver_service.LockQueue() - for !aReq.IsComplete() { - archiver_service.WaitForCompletion() - } - archiver_service.UnlockQueue() + complete = aReq.WaitForCompletion() } - if aReq.IsComplete() { + if complete { ctx.ServeFile(aReq.GetArchivePath(), downloadName) } else { ctx.Error(404) @@ -509,18 +506,7 @@ func InitiateDownload(ctx *context.Context) { complete := aReq.IsComplete() if !complete { aReq = archiver_service.ArchiveRepository(aReq) - // As with the standard Download, we'll wait up to two seconds for the request - // to be completed. The difference is that we'll never download the file from a POST - // request, only indicate the current status. If we did manage to complete the request - // in this timeframe, the download will proceed with no further overhead. - timeout := time.Now().Add(2 * time.Second) - for { - if aReq.IsComplete() || time.Now().After(timeout) { - break - } - } - - complete = aReq.IsComplete() + complete, _ = aReq.TimedWaitForCompletion(2 * time.Second) } ctx.JSON(200, map[string]interface{}{ diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index cbf70a8d300ca..dc70da9ae97cc 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -12,6 +12,7 @@ import ( "path" "strings" "sync" + "time" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/context" @@ -36,11 +37,11 @@ type ArchiveRequest struct { archiveType git.ArchiveType archiveComplete bool commit *git.Commit + cchan chan bool } var archiveInProgress []*ArchiveRequest var archiveMutex sync.Mutex -var archiveCond *sync.Cond // These facilitate testing, by allowing the unit tests to control (to some extent) // the goroutine used for processing the queue. @@ -64,6 +65,27 @@ func (aReq *ArchiveRequest) IsComplete() bool { return aReq.archiveComplete } +// WaitForCompletion will wait for this request to complete, with no timeout. +// It returns whether the archive was actually completed, as the channel could +// have also been closed due to an error. +func (aReq *ArchiveRequest) WaitForCompletion() bool { + _, _ = <-aReq.cchan + return aReq.IsComplete() +} + +// TimedWaitForCompletion will wait for this request to complete, with timeout +// happening after the specified Duration. It returns whether the archive is +// now complete and whether we hit the timeout or not. The latter may not be +// useful if the request is complete. +func (aReq *ArchiveRequest) TimedWaitForCompletion(dur time.Duration) (bool, bool) { + select { + case <-time.After(dur): + return aReq.IsComplete(), true + case <-aReq.cchan: + return aReq.IsComplete(), false + } +} + // The caller must hold the archiveMutex across calls to getArchiveRequest. func getArchiveRequest(repo *git.Repository, commit *git.Commit, archiveType git.ArchiveType) *ArchiveRequest { for _, r := range archiveInProgress { @@ -172,6 +194,7 @@ func doArchive(r *ArchiveRequest) { tmpArchive, err = ioutil.TempFile("", "archive") if err != nil { log.Error("Unable to create a temporary archive file! Error: %v", err) + close(r.cchan) return } defer func() { @@ -184,18 +207,21 @@ func doArchive(r *ArchiveRequest) { Prefix: setting.Repository.PrefixArchiveFiles, }); err != nil { log.Error("Download -> CreateArchive "+tmpArchive.Name(), err) + close(r.cchan) return } // Now we copy it into place if destArchive, err = os.Create(r.archivePath); err != nil { log.Error("Unable to open archive " + r.archivePath) + close(r.cchan) return } _, err = io.Copy(destArchive, tmpArchive) destArchive.Close() if err != nil { log.Error("Unable to write archive " + r.archivePath) + close(r.cchan) return } @@ -223,6 +249,7 @@ func ArchiveRepository(request *ArchiveRequest) *ArchiveRequest { return request } + request.cchan = make(chan bool) archiveInProgress = append(archiveInProgress, request) archiveMutex.Unlock() go func() { @@ -255,10 +282,10 @@ func ArchiveRepository(request *ArchiveRequest) *ArchiveRequest { // correctness. archiveMutex.Lock() defer archiveMutex.Unlock() - // Wake up all other goroutines that may be waiting on a request to - // complete. They should all wake up, see if that particular request - // is complete, then return to waiting if it is not. - archiveCond.Broadcast() + + // Close the channel to indicate to potential waiters that this request + // has finished. + close(request.cchan) idx := -1 for _idx, req := range archiveInProgress { @@ -280,30 +307,3 @@ func ArchiveRepository(request *ArchiveRequest) *ArchiveRequest { return request } - -// LockQueue will obtain the archiveMutex for the caller. This allows the -// underlying locking mechanism to remain opaque. -func LockQueue() { - archiveMutex.Lock() -} - -// UnlockQueue will release the archiveMutex for the caller, again allowing the -// underlying locking mechanism to remain opaque. -func UnlockQueue() { - archiveMutex.Unlock() -} - -// WaitForCompletion should be called with the queue locked (LockQueue), and will -// return with the queue lock held when a single archive request has finished. -// There is currently no API for getting notified of a particular request being -// completed. -func WaitForCompletion() { - archiveCond.Wait() -} - -// NewContext will initialize local state, e.g. primitives needed to be able to -// synchronize with the lock queue and allow callers to wait for an archive to -// finish. -func NewContext() { - archiveCond = sync.NewCond(&archiveMutex) -} From 8a501132ccaa48532a09de58e6f5b7bece98d627 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Thu, 7 May 2020 23:59:25 -0500 Subject: [PATCH 21/42] services: archiver: use defer to unlock now that we can This previously carried the lock into the goroutine, but an intermediate step just added the request to archiveInProgress outside of the new goroutine and removed the need for the goroutine to start out with it. --- services/archiver/archiver.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index dc70da9ae97cc..0d96d5e973364 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -240,18 +240,16 @@ func ArchiveRepository(request *ArchiveRequest) *ArchiveRequest { // enqueued, or we'll immediately enqueue it if it has not been enqueued // and it is not marked complete. archiveMutex.Lock() + defer archiveMutex.Unlock() if rExisting := getArchiveRequest(request.repo, request.commit, request.archiveType); rExisting != nil { - archiveMutex.Unlock() return rExisting } if request.archiveComplete { - archiveMutex.Unlock() return request } request.cchan = make(chan bool) archiveInProgress = append(archiveInProgress, request) - archiveMutex.Unlock() go func() { // Wait to start, if we have the Cond for it. This is currently only // useful for testing, so that the start and release of queued entries From 397885f9607f63cd4b26cde237a06e192d825f65 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Fri, 8 May 2020 00:01:47 -0500 Subject: [PATCH 22/42] Revert "archiver: tests: combine numQueued declaration with next-instruction assignment" This reverts commit bcc52140238e16680f2e05e448e9be51372afdf5. Revert "archiver: tests: integrate new WaitForCompletion a little better" This reverts commit 9fc8bedb5667d24d3a3c7843dc28a229efffb1e6. Revert "archiver: tests: fix test by calling NewContext()" This reverts commit 709c35685eaaf261ebbb7d3420e3376a4ee8e7f2. Revert "archiver: tests: amend test to include WaitForCompletion()" This reverts commit 75261f56bc05d1fa8ff7e81dcbc0ccd93fdc9d50. --- services/archiver/archiver_test.go | 33 ++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 5ff8c8ecf81dd..61f9e72958012 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -49,29 +49,32 @@ func waitForCount(t *testing.T, num int) { } func releaseOneEntry(t *testing.T, inFlight []*ArchiveRequest) { - numQueued := len(archiveInProgress) - - // Release one, then WaitForCompletion. We'll get signalled when ready. - // This works out to be quick and race-free, as we'll get signalled when the - // archival goroutine proceeds to dequeue the now-complete archive but we - // can't pick up the queue lock again until it's done removing it from - // archiveInProgress. We'll remain waiting on the queue lock in - // WaitForCompletion() until we can safely acquire the lock. - LockQueue() + var nowQueued, numQueued int + + numQueued = len(archiveInProgress) + + // Release one, then wait up to 10 seconds for it to complete. + queueMutex.Lock() archiveQueueReleaseCond.Signal() - WaitForCompletion() - UnlockQueue() + queueMutex.Unlock() + timeout := time.Now().Add(10 * time.Second) + for { + nowQueued = len(archiveInProgress) + if nowQueued != numQueued || time.Now().After(timeout) { + break + } + } + + // Make sure we didn't just timeout. + assert.NotEqual(t, numQueued, nowQueued) // Also make sure that we released only one. - assert.Equal(t, numQueued-1, len(archiveInProgress)) + assert.Equal(t, numQueued-1, nowQueued) } func TestArchive_Basic(t *testing.T) { assert.NoError(t, models.PrepareTestDatabase()) - // Create a new context here, because we may want to use locks or need other - // initial state here. - NewContext() archiveQueueMutex = &queueMutex archiveQueueStartCond = sync.NewCond(&queueMutex) archiveQueueReleaseCond = sync.NewCond(&queueMutex) From a6bff86df4b5f9b38fbfee6f5f15b7f29d1cb93f Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Fri, 8 May 2020 00:11:41 -0500 Subject: [PATCH 23/42] archiver: tests: first attempt at WaitForCompletion() tests --- services/archiver/archiver_test.go | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 61f9e72958012..863544bf9fe3c 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -184,9 +184,32 @@ func TestArchive_Basic(t *testing.T) { assert.Equal(t, 2, len(archiveInProgress)) releaseOneEntry(t, inFlight) assert.Equal(t, 1, len(archiveInProgress)) - releaseOneEntry(t, inFlight) + // For the last one, we'll WaitForCompletion. + finalReq := inFlight[0] + completed := finalReq.WaitForCompletion() + + assert.Equal(t, true, completed) assert.Equal(t, 0, len(archiveInProgress)) + // Now we'll submit a request and TimedWaitForCompletion twice: + // 1. With an incredibly small timeout, and + // 2. With a reasonable timeout + // We should trigger both the timeout and non-timeout cases. + var timedout bool + timedReq := DeriveRequestFrom(ctx, secondCommit+".tar.gz") + assert.NotNil(t, timedReq) + ArchiveRepository(timedReq) + + // Unreasonable: 5us + completed, timedout = timedReq.TimedWaitForCompletion(5 * time.Microsecond) + assert.Equal(t, false, completed) + assert.Equal(t, true, timedout) + + // Reasonable: 5s + completed, timedout = timedReq.TimedWaitForCompletion(5 * time.Second) + assert.Equal(t, true, completed) + assert.Equal(t, false, timedout) + zipReq2 = DeriveRequestFrom(ctx, firstCommit+".zip") // Now, we're guaranteed to have released the original zipReq from the queue. // Ensure that we don't get handed back the released entry somehow, but they From f2b3f654059bdb83f02246fc703e03931bb3cc34 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Fri, 8 May 2020 00:15:49 -0500 Subject: [PATCH 24/42] archiver: tests: slight improvement, less busy-loop Just wait for the requests to complete in order, instead of busy-waiting with a timeout. This is slightly less fragile. While here, reverse the arguments of a nearby assert.Equal() so that expected/actual are correct in any test output. --- services/archiver/archiver_test.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 863544bf9fe3c..69a9cea451696 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -146,12 +146,10 @@ func TestArchive_Basic(t *testing.T) { archiveQueueStartCond.Broadcast() queueMutex.Unlock() - // 10 second timeout for them all to complete. - timeout := time.Now().Add(10 * time.Second) - for { - if allComplete(inFlight) || time.Now().After(timeout) { - break - } + // Iterate through all of the in-flight requests and wait for their + // completion. + for _, req := range inFlight { + req.WaitForCompletion() } assert.True(t, zipReq.IsComplete()) @@ -163,7 +161,7 @@ func TestArchive_Basic(t *testing.T) { // Queues should not have drained yet, because we haven't released them. // Do so now. - assert.Equal(t, len(archiveInProgress), 3) + assert.Equal(t, 3, len(archiveInProgress)) zipReq2 := DeriveRequestFrom(ctx, firstCommit+".zip") // This zipReq should match what's sitting in the queue, as we haven't From a039d15caa92f77d262daf9776eae0078b544eb3 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Fri, 8 May 2020 00:51:52 -0500 Subject: [PATCH 25/42] archiver: address lint nits --- routers/repo/repo.go | 4 ++-- services/archiver/archiver.go | 2 +- services/archiver/archiver_test.go | 10 ---------- 3 files changed, 3 insertions(+), 13 deletions(-) diff --git a/routers/repo/repo.go b/routers/repo/repo.go index 6910bf3ceed33..db0a318b29351 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -469,7 +469,7 @@ func DownloadStatus(ctx *context.Context) { complete := aReq.IsComplete() ctx.JSON(200, map[string]interface{}{ - "complete": complete, + "complete": complete, }) } @@ -510,7 +510,7 @@ func InitiateDownload(ctx *context.Context) { } ctx.JSON(200, map[string]interface{}{ - "complete": complete, + "complete": complete, }) } diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index 0d96d5e973364..f4671a5c3c7eb 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -69,7 +69,7 @@ func (aReq *ArchiveRequest) IsComplete() bool { // It returns whether the archive was actually completed, as the channel could // have also been closed due to an error. func (aReq *ArchiveRequest) WaitForCompletion() bool { - _, _ = <-aReq.cchan + <-aReq.cchan return aReq.IsComplete() } diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 69a9cea451696..1d2e54a6db86f 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -23,16 +23,6 @@ func TestMain(m *testing.M) { models.MainTest(m, filepath.Join("..", "..")) } -func allComplete(inFlight []*ArchiveRequest) bool { - for _, req := range inFlight { - if !req.IsComplete() { - return false - } - } - - return true -} - func waitForCount(t *testing.T, num int) { var numQueued int From c96693b9f6f9fab948f43afbffa080a448aadc2c Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Fri, 8 May 2020 07:48:00 -0500 Subject: [PATCH 26/42] services: archiver: only close the channel once --- services/archiver/archiver.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index f4671a5c3c7eb..8eec68836054c 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -194,7 +194,6 @@ func doArchive(r *ArchiveRequest) { tmpArchive, err = ioutil.TempFile("", "archive") if err != nil { log.Error("Unable to create a temporary archive file! Error: %v", err) - close(r.cchan) return } defer func() { @@ -207,21 +206,18 @@ func doArchive(r *ArchiveRequest) { Prefix: setting.Repository.PrefixArchiveFiles, }); err != nil { log.Error("Download -> CreateArchive "+tmpArchive.Name(), err) - close(r.cchan) return } // Now we copy it into place if destArchive, err = os.Create(r.archivePath); err != nil { log.Error("Unable to open archive " + r.archivePath) - close(r.cchan) return } _, err = io.Copy(destArchive, tmpArchive) destArchive.Close() if err != nil { log.Error("Unable to write archive " + r.archivePath) - close(r.cchan) return } From 5deb83e0c608bc0fe3170b982c5d0ac66c2cc638 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 9 May 2020 20:29:52 -0500 Subject: [PATCH 27/42] services: archiver: use a struct{} for the wait channel This makes it obvious that the channel is only being used as a signal, rather than anything useful being piped through it. --- services/archiver/archiver.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index 8eec68836054c..361f9c69771c8 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -37,7 +37,7 @@ type ArchiveRequest struct { archiveType git.ArchiveType archiveComplete bool commit *git.Commit - cchan chan bool + cchan chan struct{} } var archiveInProgress []*ArchiveRequest @@ -244,7 +244,7 @@ func ArchiveRepository(request *ArchiveRequest) *ArchiveRequest { return request } - request.cchan = make(chan bool) + request.cchan = make(chan struct{}) archiveInProgress = append(archiveInProgress, request) go func() { // Wait to start, if we have the Cond for it. This is currently only From f831cc22bf78a8aeb69ceb9a23176916dbb5fd1d Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 9 May 2020 20:30:33 -0500 Subject: [PATCH 28/42] archiver: tests: fix expectations Move the close of the channel into doArchive() itself; notably, before these goroutines move on to waiting on the Release cond. The tests are adjusted to reflect that we can't WaitForCompletion() after they've already completed, as WaitForCompletion() doesn't indicate that they've been released from the queue yet. --- services/archiver/archiver.go | 8 ++++---- services/archiver/archiver_test.go | 8 ++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index 361f9c69771c8..da3a18d97934a 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -179,6 +179,10 @@ func doArchive(r *ArchiveRequest) { destArchive *os.File ) + // Close the channel to indicate to potential waiters that this request + // has finished. + defer close(r.cchan) + // It could have happened that we enqueued two archival requests, due to // race conditions and difficulties in locking. Do one last check that // the archive we're referring to doesn't already exist. If it does exist, @@ -277,10 +281,6 @@ func ArchiveRepository(request *ArchiveRequest) *ArchiveRequest { archiveMutex.Lock() defer archiveMutex.Unlock() - // Close the channel to indicate to potential waiters that this request - // has finished. - close(request.cchan) - idx := -1 for _idx, req := range archiveInProgress { if req == request { diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 1d2e54a6db86f..5511444368c6b 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -172,18 +172,14 @@ func TestArchive_Basic(t *testing.T) { assert.Equal(t, 2, len(archiveInProgress)) releaseOneEntry(t, inFlight) assert.Equal(t, 1, len(archiveInProgress)) - // For the last one, we'll WaitForCompletion. - finalReq := inFlight[0] - completed := finalReq.WaitForCompletion() - - assert.Equal(t, true, completed) + releaseOneEntry(t, inFlight) assert.Equal(t, 0, len(archiveInProgress)) // Now we'll submit a request and TimedWaitForCompletion twice: // 1. With an incredibly small timeout, and // 2. With a reasonable timeout // We should trigger both the timeout and non-timeout cases. - var timedout bool + var completed, timedout bool timedReq := DeriveRequestFrom(ctx, secondCommit+".tar.gz") assert.NotNil(t, timedReq) ArchiveRepository(timedReq) From 43a6b48413389d4b268dc8958eaf23bb9d48f9bd Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 9 May 2020 20:43:31 -0500 Subject: [PATCH 29/42] archiver: tests: set cchan to nil for comparison --- services/archiver/archiver_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 5511444368c6b..82f39f5bdd47b 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -197,7 +197,10 @@ func TestArchive_Basic(t *testing.T) { zipReq2 = DeriveRequestFrom(ctx, firstCommit+".zip") // Now, we're guaranteed to have released the original zipReq from the queue. // Ensure that we don't get handed back the released entry somehow, but they - // should remain functionally equivalent in all fields. + // should remain functionally equivalent in all fields. The exception here + // is zipReq.cchan, which will be non-nil because it's a completed request. + // It's fine to go ahead and set it to nil now. + zipReq.cchan = nil assert.Equal(t, zipReq, zipReq2) assert.False(t, zipReq == zipReq2) From 90bdd2c53ce3fa448fd829b2fef38c22d4acf475 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 9 May 2020 21:10:35 -0500 Subject: [PATCH 30/42] archiver: move ctx.Error's back into the route handlers We shouldn't be setting this in a service, we should just be validating the request that we were handed. --- routers/repo/repo.go | 6 ++++++ services/archiver/archiver.go | 2 -- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/routers/repo/repo.go b/routers/repo/repo.go index db0a318b29351..41a9f3960b358 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -478,6 +478,11 @@ func Download(ctx *context.Context) { uri := ctx.Params("*") aReq := archiver_service.DeriveRequestFrom(ctx, uri) + if aReq == nil { + ctx.Error(404) + return + } + downloadName := ctx.Repo.Repository.Name + "-" + aReq.GetArchiveName() complete := aReq.IsComplete() if !complete { @@ -500,6 +505,7 @@ func InitiateDownload(ctx *context.Context) { aReq := archiver_service.DeriveRequestFrom(ctx, uri) if aReq == nil { + ctx.Error(404) return } diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index da3a18d97934a..d36db7094a914 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -103,7 +103,6 @@ func getArchiveRequest(repo *git.Repository, commit *git.Commit, archiveType git func DeriveRequestFrom(ctx *context.Context, uri string) *ArchiveRequest { if ctx.Repo == nil || ctx.Repo.GitRepo == nil { log.Trace("Repo not initialized") - ctx.Error(404) return nil } r := &ArchiveRequest{ @@ -122,7 +121,6 @@ func DeriveRequestFrom(ctx *context.Context, uri string) *ArchiveRequest { r.archiveType = git.TARGZ default: log.Trace("Unknown format: %s", uri) - ctx.Error(404) return nil } From ed517bc7ff6e4dd6a6d61accd2bc755097632809 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 9 May 2020 21:11:27 -0500 Subject: [PATCH 31/42] services: archiver: use regex to match a hash This makes sure we don't try and use refName as a hash when it's clearly not one, e.g. heads/pull/foo. --- services/archiver/archiver.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index d36db7094a914..d91e3b183cafd 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -10,6 +10,7 @@ import ( "io/ioutil" "os" "path" + "regexp" "strings" "sync" "time" @@ -43,6 +44,10 @@ type ArchiveRequest struct { var archiveInProgress []*ArchiveRequest var archiveMutex sync.Mutex +// SHA1 hashes will only go up to 40 characters, but SHA256 hashes will go all +// the way to 64. +var shaRegex = regexp.MustCompile(`^[0-9a-f]{4,64}$`) + // These facilitate testing, by allowing the unit tests to control (to some extent) // the goroutine used for processing the queue. var archiveQueueMutex *sync.Mutex @@ -148,7 +153,7 @@ func DeriveRequestFrom(ctx *context.Context, uri string) *ArchiveRequest { ctx.ServerError("GetTagCommit", err) return nil } - } else if len(r.refName) >= 4 && len(r.refName) <= 40 { + } else if shaRegex.MatchString(r.refName) { r.commit, err = r.repo.GetCommit(r.refName) if err != nil { ctx.NotFound("GetCommit", nil) From d148570f9bf3003e6c881bd5d533e5669a7a1a39 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 9 May 2020 21:26:51 -0500 Subject: [PATCH 32/42] routers: repo: remove the weird /archive/status endpoint We don't need to do this anymore, we can just continue POSTing to the archive/* endpoint until we're told the download's complete. This avoids a potential naming conflict, where a ref could start with "status/" --- routers/repo/repo.go | 18 ------------- routers/routes/routes.go | 1 - templates/repo/branch/list.tmpl | 8 +++--- templates/repo/home.tmpl | 4 +-- templates/repo/release/list.tmpl | 8 +++--- web_src/js/index.js | 45 ++++---------------------------- 6 files changed, 15 insertions(+), 69 deletions(-) diff --git a/routers/repo/repo.go b/routers/repo/repo.go index 41a9f3960b358..06fc909959b4c 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -455,24 +455,6 @@ func RedirectDownload(ctx *context.Context) { ctx.Error(404) } -// DownloadStatus checks the status of a download, because archiving may take a -// while. It does so by creating an archive request from the archiver service, -// then just examining the completion status. -func DownloadStatus(ctx *context.Context) { - uri := ctx.Params("*") - aReq := archiver_service.DeriveRequestFrom(ctx, uri) - - if aReq == nil { - ctx.Error(404) - return - } - - complete := aReq.IsComplete() - ctx.JSON(200, map[string]interface{}{ - "complete": complete, - }) -} - // Download an archive of a repository func Download(ctx *context.Context) { uri := ctx.Params("*") diff --git a/routers/routes/routes.go b/routers/routes/routes.go index a82c071b6d235..cd5101a01d9d7 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -879,7 +879,6 @@ func RegisterRoutes(m *macaron.Macaron) { }, context.RepoRef(), repo.MustBeNotEmpty, context.RequireRepoReaderOr(models.UnitTypeCode)) m.Group("/archive", func() { - m.Post("/status/*", repo.DownloadStatus) m.Get("/*", repo.Download) m.Post("/*", repo.InitiateDownload) }, repo.MustBeNotEmpty, reqRepoCodeReader) diff --git a/templates/repo/branch/list.tmpl b/templates/repo/branch/list.tmpl index 4928b5d6e7cb9..297fdf6bcf49c 100644 --- a/templates/repo/branch/list.tmpl +++ b/templates/repo/branch/list.tmpl @@ -27,8 +27,8 @@
    - {{svg "octicon-file-zip" 16}} ZIP - {{svg "octicon-file-zip" 16}} TAR.GZ + {{svg "octicon-file-zip" 16}} ZIP + {{svg "octicon-file-zip" 16}} TAR.GZ
    @@ -106,8 +106,8 @@
    - {{svg "octicon-file-zip" 16}} ZIP - {{svg "octicon-file-zip" 16}} TAR.GZ + {{svg "octicon-file-zip" 16}} ZIP + {{svg "octicon-file-zip" 16}} TAR.GZ
    {{end}} diff --git a/templates/repo/home.tmpl b/templates/repo/home.tmpl index 55cc68dd1646f..a06c38cc9deee 100644 --- a/templates/repo/home.tmpl +++ b/templates/repo/home.tmpl @@ -133,8 +133,8 @@
    - {{svg "octicon-file-zip" 16}} ZIP - {{svg "octicon-file-zip" 16}} TAR.GZ + {{svg "octicon-file-zip" 16}} ZIP + {{svg "octicon-file-zip" 16}} TAR.GZ
    diff --git a/templates/repo/release/list.tmpl b/templates/repo/release/list.tmpl index d13fefe8be49b..e49e2d51ff6f7 100644 --- a/templates/repo/release/list.tmpl +++ b/templates/repo/release/list.tmpl @@ -43,8 +43,8 @@
    {{if $.Permission.CanRead $.UnitTypeCode}} {{ShortSha .Sha1}} - {{svg "octicon-file-zip" 16}} ZIP - {{svg "octicon-file-zip" 16}} TAR.GZ + {{svg "octicon-file-zip" 16}} ZIP + {{svg "octicon-file-zip" 16}} TAR.GZ {{end}}
    {{else}} @@ -80,10 +80,10 @@
      {{if $.Permission.CanRead $.UnitTypeCode}}
    • - {{svg "octicon-file-zip" 16}} {{$.i18n.Tr "repo.release.source_code"}} (ZIP) + {{svg "octicon-file-zip" 16}} {{$.i18n.Tr "repo.release.source_code"}} (ZIP)
    • - {{svg "octicon-file-zip" 16}} {{$.i18n.Tr "repo.release.source_code"}} (TAR.GZ) + {{svg "octicon-file-zip" 16}} {{$.i18n.Tr "repo.release.source_code"}} (TAR.GZ)
      • {{end}} {{if .Attachments}} diff --git a/web_src/js/index.js b/web_src/js/index.js index 45237fce14d74..88c497bb1c553 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -698,38 +698,7 @@ function initIssueComments() { }); } -function initArchiveStatusChecker($target, url, statusUrl) { - $.ajax({ - url: statusUrl, - type: 'POST', - data: { - _csrf: csrf, - }, - complete(xhr) { - if (xhr.status === 200) { - if (!xhr.responseJSON) { - $target.closest('.dropdown').children('i').removeClass('loading'); - return; - } - - if (xhr.responseJSON.complete) { - // Null out the status URL. We don't need to query status again. - // getArchive() will clear the loading indicator here, as needed. - getArchive($target, url, null); - return; - } - - setTimeout(() => { - initArchiveStatusChecker($target, url, statusUrl); - }, 2000); - } else { - $target.closest('.dropdown').children('i').removeClass('loading'); - } - } - }); -} - -function getArchive($target, url, statusUrl) { +function getArchive($target, url, first) { $.ajax({ url, type: 'POST', @@ -744,13 +713,13 @@ function getArchive($target, url, statusUrl) { return; } - if (!xhr.responseJSON.complete && statusUrl !== null) { + if (!xhr.responseJSON.complete) { $target.closest('.dropdown').children('i').addClass('loading'); // Wait for only three quarters of a second initially, in case it's // quickly archived. setTimeout(() => { - initArchiveStatusChecker($target, url, statusUrl); - }, 750); + getArchive($target, url, false); + }, first ? 750 : 2000); } else { // We don't need to continue checking. $target.closest('.dropdown').children('i').removeClass('loading'); @@ -771,12 +740,8 @@ function initArchiveLinks() { if (typeof url === 'undefined') { return; } - const statusUrl = $(this).data('status'); - if (typeof statusUrl === 'undefined') { - return; - } - getArchive($(event.target), url, statusUrl); + getArchive($(event.target), url, true); }); } From b81fa9b4ccfc0836021399f82000d8ed973a1299 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 9 May 2020 21:52:26 -0500 Subject: [PATCH 33/42] archiver: tests: bump reasonable timeout to 15s --- services/archiver/archiver_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 82f39f5bdd47b..1b1c32a31f5ac 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -189,8 +189,8 @@ func TestArchive_Basic(t *testing.T) { assert.Equal(t, false, completed) assert.Equal(t, true, timedout) - // Reasonable: 5s - completed, timedout = timedReq.TimedWaitForCompletion(5 * time.Second) + // Reasonable: 15s + completed, timedout = timedReq.TimedWaitForCompletion(15 * time.Second) assert.Equal(t, true, completed) assert.Equal(t, false, timedout) From 82c1dabeec09d0ce652c88eef9fe7193a0aa4f36 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 9 May 2020 23:33:59 -0500 Subject: [PATCH 34/42] archiver: tests: actually release timedReq --- services/archiver/archiver_test.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 1b1c32a31f5ac..856b2576c14f2 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -175,21 +175,24 @@ func TestArchive_Basic(t *testing.T) { releaseOneEntry(t, inFlight) assert.Equal(t, 0, len(archiveInProgress)) - // Now we'll submit a request and TimedWaitForCompletion twice: - // 1. With an incredibly small timeout, and - // 2. With a reasonable timeout - // We should trigger both the timeout and non-timeout cases. + // Now we'll submit a request and TimedWaitForCompletion twice, before and + // after we release it. We should trigger both the timeout and non-timeout + // cases. var completed, timedout bool timedReq := DeriveRequestFrom(ctx, secondCommit+".tar.gz") assert.NotNil(t, timedReq) ArchiveRepository(timedReq) - // Unreasonable: 5us - completed, timedout = timedReq.TimedWaitForCompletion(5 * time.Microsecond) + // Guaranteed to timeout; we haven't signalled the request to start.. + completed, timedout = timedReq.TimedWaitForCompletion(2 * time.Second) assert.Equal(t, false, completed) assert.Equal(t, true, timedout) - // Reasonable: 15s + queueMutex.Lock() + archiveQueueStartCond.Broadcast() + queueMutex.Unlock() + + // Shouldn't timeout, we've now signalled it and it's a small request. completed, timedout = timedReq.TimedWaitForCompletion(15 * time.Second) assert.Equal(t, true, completed) assert.Equal(t, false, timedout) From 091d05354bd8803043020a32c9bd4cdf943beeec Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 9 May 2020 23:51:09 -0500 Subject: [PATCH 35/42] archiver: tests: run through inFlight instead of manually checking While we're here, add a test for manually re-processing an archive that's already been complete. Re-open the channel and mark it incomplete, so that doArchive can just mark it complete again. --- services/archiver/archiver_test.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 856b2576c14f2..63b56e543a8d8 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -142,12 +142,19 @@ func TestArchive_Basic(t *testing.T) { req.WaitForCompletion() } - assert.True(t, zipReq.IsComplete()) - assert.True(t, tgzReq.IsComplete()) - assert.True(t, secondReq.IsComplete()) - assert.True(t, com.IsExist(zipReq.GetArchivePath())) - assert.True(t, com.IsExist(tgzReq.GetArchivePath())) - assert.True(t, com.IsExist(secondReq.GetArchivePath())) + for _, req := range inFlight { + assert.True(t, req.IsComplete()) + assert.True(t, com.IsExist(req.GetArchivePath())) + } + + arbitraryReq := inFlight[0] + // Reopen the channel so we don't double-close, mark it incomplete. We're + // going to run it back through the archiver, and it should get marked + // complete again. + arbitraryReq.cchan = make(chan struct{}) + arbitraryReq.archiveComplete = false + doArchive(arbitraryReq) + assert.True(t, arbitraryReq.IsComplete()) // Queues should not have drained yet, because we haven't released them. // Do so now. From 5417cdd810aed0b3c165da85a4f088ff2e3622ad Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Wed, 12 Aug 2020 21:17:31 -0500 Subject: [PATCH 36/42] initArchiveLinks: prevent default behavior from clicking --- web_src/js/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/web_src/js/index.js b/web_src/js/index.js index 62c94c6c81faf..5c6ab982ee28f 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -734,6 +734,7 @@ function initArchiveLinks() { return; } + event.preventDefault(); getArchive($(event.target), url, true); }); } From 081032a7a0a0ee86de1c0ba4141a5f75b77ab67d Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Wed, 12 Aug 2020 22:26:26 -0500 Subject: [PATCH 37/42] archiver: alias gitea's context, golang context import pending --- services/archiver/archiver.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index d91e3b183cafd..a0668ec131f5b 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -16,7 +16,7 @@ import ( "time" "code.gitea.io/gitea/modules/base" - "code.gitea.io/gitea/modules/context" + gitea_context "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -105,7 +105,7 @@ func getArchiveRequest(repo *git.Repository, commit *git.Commit, archiveType git // DeriveRequestFrom creates an archival request, based on the URI. The // resulting ArchiveRequest is suitable for being passed to ArchiveRepository() // if it's determined that the request still needs to be satisfied. -func DeriveRequestFrom(ctx *context.Context, uri string) *ArchiveRequest { +func DeriveRequestFrom(ctx *gitea_context.Context, uri string) *ArchiveRequest { if ctx.Repo == nil || ctx.Repo.GitRepo == nil { log.Trace("Repo not initialized") return nil From eca24cdf6e10f9874a8de01d52a384af9cf188a7 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Wed, 12 Aug 2020 22:26:48 -0500 Subject: [PATCH 38/42] archiver: simplify logic, just reconstruct slices While the previous logic was perhaps slightly more efficient, the new variant's readability is much improved. --- services/archiver/archiver.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index a0668ec131f5b..97579a9b071a1 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -295,11 +295,7 @@ func ArchiveRepository(request *ArchiveRequest) *ArchiveRequest { log.Error("ArchiveRepository: Failed to find request for removal.") return } - lastidx := len(archiveInProgress) - 1 - if idx != lastidx { - archiveInProgress[idx] = archiveInProgress[lastidx] - } - archiveInProgress = archiveInProgress[:lastidx] + archiveInProgress = append(archiveInProgress[:idx], archiveInProgress[idx+1:]...) }() return request From 57de90d73f0a6dbac1d1074c6243e27361f195f8 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Wed, 12 Aug 2020 22:28:55 -0500 Subject: [PATCH 39/42] archiver: don't block shutdown on waiting for archive The technique established launches a goroutine to do the wait, which will close a wait channel upon termination. For the timeout case, we also send back a value indicating whether the timeout was hit or not. The timeouts are expected to be relatively small, but still a multi- second delay to shutdown due to this could be unfortunate. --- services/archiver/archiver.go | 38 +++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index 97579a9b071a1..8ba9dc14f7556 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -6,6 +6,7 @@ package archiver import ( + "context" "io" "io/ioutil" "os" @@ -18,6 +19,7 @@ import ( "code.gitea.io/gitea/modules/base" gitea_context "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -74,21 +76,41 @@ func (aReq *ArchiveRequest) IsComplete() bool { // It returns whether the archive was actually completed, as the channel could // have also been closed due to an error. func (aReq *ArchiveRequest) WaitForCompletion() bool { - <-aReq.cchan + compchan := make(chan struct{}) + go graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { + defer close(compchan) + select { + case <-aReq.cchan: + case <-ctx.Done(): + } + }) + + // Callback will always close compchan upon return, whether we're shutting + // down or not. + <-compchan return aReq.IsComplete() } // TimedWaitForCompletion will wait for this request to complete, with timeout // happening after the specified Duration. It returns whether the archive is // now complete and whether we hit the timeout or not. The latter may not be -// useful if the request is complete. +// useful if the request is complete or we started to shutdown. func (aReq *ArchiveRequest) TimedWaitForCompletion(dur time.Duration) (bool, bool) { - select { - case <-time.After(dur): - return aReq.IsComplete(), true - case <-aReq.cchan: - return aReq.IsComplete(), false - } + compchan := make(chan bool) + go graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { + defer close(compchan) + select { + case <-time.After(dur): + compchan <- true + case <-aReq.cchan: + compchan <- false + case <-ctx.Done(): + compchan <- false + } + }) + + timeout := <-compchan + return aReq.IsComplete(), timeout } // The caller must hold the archiveMutex across calls to getArchiveRequest. From 58a55c67fc08987b7e4a1771eb6efe4c0987af97 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Fri, 14 Aug 2020 10:23:36 -0500 Subject: [PATCH 40/42] archiver: simplify shutdown logic We can just grab the shutdown channel from the graceful manager instead of constructing a channel to halt the caller and/or pass a result back. --- services/archiver/archiver.go | 40 ++++++++++++----------------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/services/archiver/archiver.go b/services/archiver/archiver.go index 8ba9dc14f7556..673fbdd12fe60 100644 --- a/services/archiver/archiver.go +++ b/services/archiver/archiver.go @@ -6,7 +6,6 @@ package archiver import ( - "context" "io" "io/ioutil" "os" @@ -17,7 +16,7 @@ import ( "time" "code.gitea.io/gitea/modules/base" - gitea_context "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" @@ -76,18 +75,11 @@ func (aReq *ArchiveRequest) IsComplete() bool { // It returns whether the archive was actually completed, as the channel could // have also been closed due to an error. func (aReq *ArchiveRequest) WaitForCompletion() bool { - compchan := make(chan struct{}) - go graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { - defer close(compchan) - select { - case <-aReq.cchan: - case <-ctx.Done(): - } - }) + select { + case <-aReq.cchan: + case <-graceful.GetManager().IsShutdown(): + } - // Callback will always close compchan upon return, whether we're shutting - // down or not. - <-compchan return aReq.IsComplete() } @@ -96,20 +88,14 @@ func (aReq *ArchiveRequest) WaitForCompletion() bool { // now complete and whether we hit the timeout or not. The latter may not be // useful if the request is complete or we started to shutdown. func (aReq *ArchiveRequest) TimedWaitForCompletion(dur time.Duration) (bool, bool) { - compchan := make(chan bool) - go graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { - defer close(compchan) - select { - case <-time.After(dur): - compchan <- true - case <-aReq.cchan: - compchan <- false - case <-ctx.Done(): - compchan <- false - } - }) + timeout := false + select { + case <-time.After(dur): + timeout = true + case <-aReq.cchan: + case <-graceful.GetManager().IsShutdown(): + } - timeout := <-compchan return aReq.IsComplete(), timeout } @@ -127,7 +113,7 @@ func getArchiveRequest(repo *git.Repository, commit *git.Commit, archiveType git // DeriveRequestFrom creates an archival request, based on the URI. The // resulting ArchiveRequest is suitable for being passed to ArchiveRepository() // if it's determined that the request still needs to be satisfied. -func DeriveRequestFrom(ctx *gitea_context.Context, uri string) *ArchiveRequest { +func DeriveRequestFrom(ctx *context.Context, uri string) *ArchiveRequest { if ctx.Repo == nil || ctx.Repo.GitRepo == nil { log.Trace("Repo not initialized") return nil From 8c2afd701f9fca76977e0ad4b993c64b9471187e Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Fri, 11 Sep 2020 15:31:42 -0500 Subject: [PATCH 41/42] Style issues --- routers/repo/repo.go | 2 +- services/archiver/archiver_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/routers/repo/repo.go b/routers/repo/repo.go index f0e93eeb5ded9..b3b4f7ec96d6f 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -348,7 +348,7 @@ func InitiateDownload(ctx *context.Context) { complete := aReq.IsComplete() if !complete { aReq = archiver_service.ArchiveRepository(aReq) - complete, _ = aReq.TimedWaitForCompletion(ctx, 2 * time.Second) + complete, _ = aReq.TimedWaitForCompletion(ctx, 2*time.Second) } ctx.JSON(200, map[string]interface{}{ diff --git a/services/archiver/archiver_test.go b/services/archiver/archiver_test.go index 377ea76133fdb..8a0f73bf9c460 100644 --- a/services/archiver/archiver_test.go +++ b/services/archiver/archiver_test.go @@ -191,7 +191,7 @@ func TestArchive_Basic(t *testing.T) { ArchiveRepository(timedReq) // Guaranteed to timeout; we haven't signalled the request to start.. - completed, timedout = timedReq.TimedWaitForCompletion(ctx, 2 * time.Second) + completed, timedout = timedReq.TimedWaitForCompletion(ctx, 2*time.Second) assert.Equal(t, false, completed) assert.Equal(t, true, timedout) @@ -200,7 +200,7 @@ func TestArchive_Basic(t *testing.T) { queueMutex.Unlock() // Shouldn't timeout, we've now signalled it and it's a small request. - completed, timedout = timedReq.TimedWaitForCompletion(ctx, 15 * time.Second) + completed, timedout = timedReq.TimedWaitForCompletion(ctx, 15*time.Second) assert.Equal(t, true, completed) assert.Equal(t, false, timedout) From e303a5b5f992a0521b2284f22f96d6f72e054de5 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 7 Nov 2020 10:16:04 -0600 Subject: [PATCH 42/42] Fix mis-merge --- templates/repo/release/list.tmpl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/templates/repo/release/list.tmpl b/templates/repo/release/list.tmpl index e5bf5f67c1a73..71d40fdb238f7 100644 --- a/templates/repo/release/list.tmpl +++ b/templates/repo/release/list.tmpl @@ -37,8 +37,8 @@
      {{if $.Permission.CanRead $.UnitTypeCode}} {{svg "octicon-git-commit" 16 "mr-2"}}{{ShortSha .Sha1}} - {{svg "octicon-file-zip" 16 "mr-2"}}ZIP - {{svg "octicon-file-zip" 16 "mr-2"}}TAR.GZ + {{svg "octicon-file-zip" 16 "mr-2"}}ZIP + {{svg "octicon-file-zip" 16 "mr-2"}}TAR.GZ {{if (and $.CanCreateRelease $release.IsTag)}} {{svg "octicon-tag" 16 "mr-2"}}{{$.i18n.Tr "repo.release.new_release"}} {{end}}