Re-attempt to delete temporary upload if the file is locked by another process (#12447)

Replace all calls to os.Remove/os.RemoveAll by retrying util.Remove/util.RemoveAll and remove circular dependencies from util.

Fix #12339

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: silverwind <me@silverwind.io>

4 files changed, 57 insertions, 56 deletions

diff --git a/modules/util/remove.go b/modules/util/remove.go
new file mode 100644
index 0000000000..f2bbbc30b9
--- /dev/null
+++ b/modules/util/remove.go
@@ -0,0 +1,57 @@
+// 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 util
+
+import (
+	"os"
+	"syscall"
+	"time"
+)
+
+// Remove removes the named file or (empty) directory with at most 5 attempts.
+func Remove(name string) error {
+	var err error
+	for i := 0; i < 5; i++ {
+		err = os.Remove(name)
+		if err == nil {
+			break
+		}
+		unwrapped := err.(*os.PathError).Err
+		if unwrapped == syscall.EBUSY || unwrapped == syscall.ENOTEMPTY || unwrapped == syscall.EPERM || unwrapped == syscall.EMFILE || unwrapped == syscall.ENFILE {
+			// try again
+			<-time.After(100 * time.Millisecond)
+			continue
+		}
+
+		if unwrapped == syscall.ENOENT {
+			// it's already gone
+			return nil
+		}
+	}
+	return err
+}
+
+// RemoveAll removes the named file or (empty) directory with at most 5 attempts.Remove
+func RemoveAll(name string) error {
+	var err error
+	for i := 0; i < 5; i++ {
+		err = os.RemoveAll(name)
+		if err == nil {
+			break
+		}
+		unwrapped := err.(*os.PathError).Err
+		if unwrapped == syscall.EBUSY || unwrapped == syscall.ENOTEMPTY || unwrapped == syscall.EPERM || unwrapped == syscall.EMFILE || unwrapped == syscall.ENFILE {
+			// try again
+			<-time.After(100 * time.Millisecond)
+			continue
+		}
+
+		if unwrapped == syscall.ENOENT {
+			// it's already gone
+			return nil
+		}
+	}
+	return err
+}
diff --git a/modules/util/sanitize.go b/modules/util/sanitize.go
index d04e1dee77..b1c17b29cf 100644
--- a/modules/util/sanitize.go
+++ b/modules/util/sanitize.go
@@ -7,8 +7,6 @@ package util
 import (
 	"net/url"
 	"strings"
-
-	"code.gitea.io/gitea/modules/log"
 )
 
 // urlSafeError wraps an error whose message may contain a sensitive URL
@@ -38,7 +36,6 @@ func SanitizeMessage(message, unsanitizedURL string) string {
 func SanitizeURLCredentials(unsanitizedURL string, usePlaceholder bool) string {
 	u, err := url.Parse(unsanitizedURL)
 	if err != nil {
-		log.Error("parse url %s failed: %v", unsanitizedURL, err)
 		// don't log the error, since it might contain unsanitized URL.
 		return "(unparsable url)"
 	}
diff --git a/modules/util/url.go b/modules/util/url.go
index 263255fcd3..96c66df92b 100644
--- a/modules/util/url.go
+++ b/modules/util/url.go
@@ -8,9 +8,6 @@ import (
 	"net/url"
 	"path"
 	"strings"
-
-	"code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/setting"
 )
 
 // PathEscapeSegments escapes segments of a path while not escaping forward slash
@@ -30,13 +27,11 @@ func URLJoin(base string, elems ...string) string {
 	}
 	baseURL, err := url.Parse(base)
 	if err != nil {
-		log.Error("URLJoin: Invalid base URL %s", base)
 		return ""
 	}
 	joinedPath := path.Join(elems...)
 	argURL, err := url.Parse(joinedPath)
 	if err != nil {
-		log.Error("URLJoin: Invalid arg %s", joinedPath)
 		return ""
 	}
 	joinedURL := baseURL.ResolveReference(argURL).String()
@@ -45,16 +40,3 @@ func URLJoin(base string, elems ...string) string {
 	}
 	return joinedURL
 }
-
-// IsExternalURL checks if rawURL points to an external URL like http://example.com
-func IsExternalURL(rawURL string) bool {
-	parsed, err := url.Parse(rawURL)
-	if err != nil {
-		return true
-	}
-	appURL, _ := url.Parse(setting.AppURL)
-	if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(appURL.Host, "www.", "", 1) {
-		return true
-	}
-	return false
-}
diff --git a/modules/util/util_test.go b/modules/util/util_test.go
index 04ab42f292..1d4f23de90 100644
--- a/modules/util/util_test.go
+++ b/modules/util/util_test.go
@@ -8,8 +8,6 @@ import (
 	"strings"
 	"testing"
 
-	"code.gitea.io/gitea/modules/setting"
-
 	"github.com/stretchr/testify/assert"
 )
 
@@ -46,39 +44,6 @@ func TestURLJoin(t *testing.T) {
 	}
 }
 
-func TestIsExternalURL(t *testing.T) {
-	setting.AppURL = "https://try.gitea.io"
-	type test struct {
-		Expected bool
-		RawURL   string
-	}
-	newTest := func(expected bool, rawURL string) test {
-		return test{Expected: expected, RawURL: rawURL}
-	}
-	for _, test := range []test{
-		newTest(false,
-			"https://try.gitea.io"),
-		newTest(true,
-			"https://example.com/"),
-		newTest(true,
-			"//example.com"),
-		newTest(true,
-			"http://example.com"),
-		newTest(false,
-			"a/"),
-		newTest(false,
-			"https://try.gitea.io/test?param=false"),
-		newTest(false,
-			"test?param=false"),
-		newTest(false,
-			"//try.gitea.io/test?param=false"),
-		newTest(false,
-			"/hey/hey/hey#3244"),
-	} {
-		assert.Equal(t, test.Expected, IsExternalURL(test.RawURL))
-	}
-}
-
 func TestIsEmptyString(t *testing.T) {
 
 	cases := []struct {