From 248389ea6354cc59fecb51e3f6b66418e66d9acd Mon Sep 17 00:00:00 2001
From: Krameli <philipp@huschauer.de>
Date: Thu, 14 Mar 2024 01:00:00 +0100
Subject: [PATCH] Guard urls during a mirrorlist refresh (#103)

Fix #92 by locking access to the Mirrorlist's Urls during refresh.

I had the same problem on my deployed pacoloco and also successfully reproduced the issue using @chennin's script. Using this PR, both were unable to reproduce the error on my system.

I'm new to go and it's concurrency model, but my guess is that the repo archlinux has no urls error happens due to a race condition in getMirrorlistURLs():
While the Mirrorlist File is read for the first time (e.g. because of a request for 'core.db') the URLs array is still empty but LastMirrorlistCheck is already set. When in this moment the function is called concurrently (e.g. because of a parallel download request for 'extra.db') it returns the still empty URLs array due to a recent LastMirrorlistCheck, instead of waiting for it to be populated.

I used a mutex instead of setting the LastMirrorlistCheck after URLs is populated to block multiple goroutines from reading and parsing the same Mirrorlist simultaneously.
However this doesn't fix any possible racing with reflector or similar.

Closes #92
---
 config.go     | 14 ++++++++------
 downloader.go |  5 +++--
 urls.go       | 16 ++++++++++++++--
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/config.go b/config.go
index 12fc124..dee054e 100644
--- a/config.go
+++ b/config.go
@@ -3,6 +3,7 @@ package main
 import (
 	"log"
 	"os"
+	"sync"
 	"time"
 
 	"github.com/gorhill/cronexpr"
@@ -19,12 +20,13 @@ const (
 )
 
 type Repo struct {
-	URL                  string    `yaml:"url"`
-	URLs                 []string  `yaml:"urls"`
-	Mirrorlist           string    `yaml:"mirrorlist"`
-	HttpProxy            string    `yaml:"http_proxy"`
-	LastMirrorlistCheck  time.Time `yaml:"-"`
-	LastModificationTime time.Time `yaml:"-"`
+	URL                  string     `yaml:"url"`
+	URLs                 []string   `yaml:"urls"`
+	Mirrorlist           string     `yaml:"mirrorlist"`
+	HttpProxy            string     `yaml:"http_proxy"`
+	LastMirrorlistCheck  time.Time  `yaml:"-"`
+	MirrorlistMutex      sync.Mutex `yaml:"-"`
+	LastModificationTime time.Time  `yaml:"-"`
 }
 
 type RefreshPeriod struct {
diff --git a/downloader.go b/downloader.go
index b22b1d0..a5ee38a 100644
--- a/downloader.go
+++ b/downloader.go
@@ -59,7 +59,8 @@ func (d *Downloader) decrementUsage() {
 }
 
 func (d *Downloader) download() error {
-	if len(d.repo.getUrls()) == 0 {
+	urls := d.repo.getUrls()
+	if len(urls) == 0 {
 		return fmt.Errorf("repo %v has no urls", d.repoName)
 	}
 
@@ -70,7 +71,7 @@ func (d *Downloader) download() error {
 		proxyURL = nil
 	}
 
-	for _, u := range d.repo.getUrls() {
+	for _, u := range urls {
 		err := d.downloadFromUpstream(u, proxyURL)
 		if err != nil {
 			log.Printf("unable to download file %v: %v", d.key, err)
diff --git a/urls.go b/urls.go
index 1ebaab8..39c531f 100644
--- a/urls.go
+++ b/urls.go
@@ -44,11 +44,23 @@ func parseMirrorlistURLs(file *os.File) ([]string, error) {
 }
 
 func (r *Repo) getMirrorlistURLs() ([]string, error) {
-	if time.Since(r.LastMirrorlistCheck) < 5*time.Second {
+	const MirrorlistCheckInterval = 5*time.Second
+
+	if time.Since(r.LastMirrorlistCheck) < MirrorlistCheckInterval {
+		return r.URLs, nil
+	}
+
+	r.MirrorlistMutex.Lock()
+	defer r.MirrorlistMutex.Unlock()
+
+	// Test time again in case another routine already checked in the meantime
+	if time.Since(r.LastMirrorlistCheck) < MirrorlistCheckInterval {
 		return r.URLs, nil
 	}
 
-	r.LastMirrorlistCheck = time.Now()
+	defer func() {
+		r.LastMirrorlistCheck = time.Now()
+	}()
 
 	fileInfo, err := os.Stat(r.Mirrorlist)
 	if err != nil {