From f487ff03717af41247fd9d6b96d6d4254d8eb4ac Mon Sep 17 00:00:00 2001 From: Justin Hawkins Date: Thu, 6 Jan 2022 16:19:22 +1030 Subject: [PATCH] Use Process.Kill instead which is (hopefully) cross-platform enough. Improve test reliability. --- download/download.go | 19 ++++++++++--------- download/download_test.go | 17 +++++++++++++---- main.go | 1 - 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/download/download.go b/download/download.go index 4f3eba7..ee0f5ac 100644 --- a/download/download.go +++ b/download/download.go @@ -5,12 +5,12 @@ import ( "io" "log" "net/url" + "os" "os/exec" "regexp" "strconv" "strings" "sync" - "syscall" "time" "github.com/tardisx/gropple/config" @@ -20,7 +20,7 @@ type Download struct { Id int `json:"id"` Url string `json:"url"` PopupUrl string `json:"popup_url"` - Pid int `json:"pid"` + Process *os.Process `json:"-"` ExitCode int `json:"exit_code"` State string `json:"state"` DownloadProfile config.DownloadProfile `json:"download_profile"` @@ -59,7 +59,7 @@ func (dls Downloads) StartQueued(maxRunning int) { if dl.State == "queued" && (maxRunning == 0 || active[dl.domain()] < maxRunning) { dl.State = "downloading" active[dl.domain()]++ - log.Printf("Starting download for %#v", dl) + log.Printf("Starting download for id:%d (%s)", dl.Id, dl.Url) dl.mutex.Unlock() go func() { dl.Begin() }() } else { @@ -101,9 +101,9 @@ func (dl *Download) Queue() { func (dl *Download) Stop() { log.Printf("stopping the download") dl.mutex.Lock() + dl.Log = append(dl.Log, "aborted by user") defer dl.mutex.Unlock() - - syscall.Kill(dl.Pid, syscall.SIGTERM) + dl.Process.Kill() } func (dl *Download) domain() string { @@ -129,8 +129,8 @@ func (dl *Download) Begin() { cmdSlice := []string{} cmdSlice = append(cmdSlice, dl.DownloadProfile.Args...) - // only add the url if it's not empty. This helps us with testing - if dl.Url != "" { + // only add the url if it's not empty or an example URL. This helps us with testing + if !(dl.Url == "" || strings.Contains(dl.domain(), "example.org")) { cmdSlice = append(cmdSlice, dl.Url) } @@ -155,7 +155,7 @@ func (dl *Download) Begin() { return } - log.Printf("Starting %v", cmd) + log.Printf("Executing command: %v", cmd) err = cmd.Start() if err != nil { dl.State = "failed" @@ -164,7 +164,7 @@ func (dl *Download) Begin() { dl.Log = append(dl.Log, fmt.Sprintf("error starting command '%s': %v", dl.DownloadProfile.Command, err)) return } - dl.Pid = cmd.Process.Pid + dl.Process = cmd.Process var wg sync.WaitGroup @@ -185,6 +185,7 @@ func (dl *Download) Begin() { cmd.Wait() dl.mutex.Lock() + log.Printf("Process finished for id: %d (%v)", dl.Id, cmd) dl.State = "complete" dl.Finished = true diff --git a/download/download_test.go b/download/download_test.go index aa0f2b4..a949fab 100644 --- a/download/download_test.go +++ b/download/download_test.go @@ -2,6 +2,7 @@ package download import ( "testing" + "time" "github.com/tardisx/gropple/config" ) @@ -68,13 +69,14 @@ func TestUpdateMetadata(t *testing.T) { func TestQueue(t *testing.T) { conf := config.TestConfig() - new1 := Download{Id: 1, Url: "http://domain1.com/foo", State: "queued", DownloadProfile: conf.DownloadProfiles[0], Config: conf} - new2 := Download{Id: 2, Url: "http://domain1.com/foo", State: "queued", DownloadProfile: conf.DownloadProfiles[0], Config: conf} - new3 := Download{Id: 3, Url: "http://domain1.com/foo", State: "queued", DownloadProfile: conf.DownloadProfiles[0], Config: conf} - new4 := Download{Id: 4, Url: "http://company.org/", State: "queued", DownloadProfile: conf.DownloadProfiles[0], Config: conf} + new1 := Download{Id: 1, Url: "http://sub.example.org/foo1", State: "queued", DownloadProfile: conf.DownloadProfiles[0], Config: conf} + new2 := Download{Id: 2, Url: "http://sub.example.org/foo2", State: "queued", DownloadProfile: conf.DownloadProfiles[0], Config: conf} + new3 := Download{Id: 3, Url: "http://sub.example.org/foo3", State: "queued", DownloadProfile: conf.DownloadProfiles[0], Config: conf} + new4 := Download{Id: 4, Url: "http://example.org/", State: "queued", DownloadProfile: conf.DownloadProfiles[0], Config: conf} dls := Downloads{&new1, &new2, &new3, &new4} dls.StartQueued(1) + time.Sleep(time.Millisecond * 100) if dls[0].State == "queued" { t.Error("#1 was not started") } @@ -87,16 +89,20 @@ func TestQueue(t *testing.T) { // this should start no more, as one is still going dls.StartQueued(1) + time.Sleep(time.Millisecond * 100) if dls[1].State != "queued" { t.Error("#2 was started when it should not be") } dls.StartQueued(2) + time.Sleep(time.Millisecond * 100) if dls[1].State == "queued" { t.Error("#2 was not started but it should be") + } dls.StartQueued(2) + time.Sleep(time.Millisecond * 100) if dls[3].State == "queued" { t.Error("#4 was not started but it should be") } @@ -108,6 +114,7 @@ func TestQueue(t *testing.T) { dls[3].State = "queued" dls.StartQueued(0) + time.Sleep(time.Millisecond * 100) // they should all be going if dls[0].State == "queued" || dls[1].State == "queued" || dls[2].State == "queued" || dls[3].State == "queued" { @@ -121,10 +128,12 @@ func TestQueue(t *testing.T) { dls[3].State = "queued" dls.StartQueued(2) + time.Sleep(time.Millisecond * 100) // first two should be running, third not (same domain) and 4th running (different domain) if dls[0].State == "queued" || dls[1].State == "queued" || dls[2].State != "queued" || dls[3].State == "queued" { t.Error("incorrect queued") + } } diff --git a/main.go b/main.go index 090019f..efe8951 100644 --- a/main.go +++ b/main.go @@ -258,7 +258,6 @@ func fetchInfoOneRESTHandler(w http.ResponseWriter, r *http.Request) { } if thisReq.Action == "stop" { - thisDownload.Stop() succRes := successResponse{Success: true, Message: "download stopped"} succResB, _ := json.Marshal(succRes)