Browse Source

Refactor process package and introduce ProcessManager{} with tests (#75)

* Add a process.Manager singleton with process.GetManager()

* Use process.GetManager everywhere

* Fix godoc comments for process module

* Increment process counter id after locking the mutex
Matthias Loibl 3 years ago
parent
commit
d1006150fb

+ 2 - 2
models/git_diff.go

@@ -483,8 +483,8 @@ func GetDiffRange(repoPath, beforeCommitID, afterCommitID string, maxLines, maxL
483 483
 		return nil, fmt.Errorf("Start: %v", err)
484 484
 	}
485 485
 
486
-	pid := process.Add(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath), cmd)
487
-	defer process.Remove(pid)
486
+	pid := process.GetManager().Add(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath), cmd)
487
+	defer process.GetManager().Remove(pid)
488 488
 
489 489
 	diff, err := ParsePatch(maxLines, maxLineCharacters, maxFiles, stdout)
490 490
 	if err != nil {

+ 9 - 9
models/pull.go

@@ -281,41 +281,41 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository) (err error
281 281
 	defer os.RemoveAll(path.Dir(tmpBasePath))
282 282
 
283 283
 	var stderr string
284
-	if _, stderr, err = process.ExecTimeout(5*time.Minute,
284
+	if _, stderr, err = process.GetManager().ExecTimeout(5*time.Minute,
285 285
 		fmt.Sprintf("PullRequest.Merge (git clone): %s", tmpBasePath),
286 286
 		"git", "clone", baseGitRepo.Path, tmpBasePath); err != nil {
287 287
 		return fmt.Errorf("git clone: %s", stderr)
288 288
 	}
289 289
 
290 290
 	// Check out base branch.
291
-	if _, stderr, err = process.ExecDir(-1, tmpBasePath,
291
+	if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
292 292
 		fmt.Sprintf("PullRequest.Merge (git checkout): %s", tmpBasePath),
293 293
 		"git", "checkout", pr.BaseBranch); err != nil {
294 294
 		return fmt.Errorf("git checkout: %s", stderr)
295 295
 	}
296 296
 
297 297
 	// Add head repo remote.
298
-	if _, stderr, err = process.ExecDir(-1, tmpBasePath,
298
+	if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
299 299
 		fmt.Sprintf("PullRequest.Merge (git remote add): %s", tmpBasePath),
300 300
 		"git", "remote", "add", "head_repo", headRepoPath); err != nil {
301 301
 		return fmt.Errorf("git remote add [%s -> %s]: %s", headRepoPath, tmpBasePath, stderr)
302 302
 	}
303 303
 
304 304
 	// Merge commits.
305
-	if _, stderr, err = process.ExecDir(-1, tmpBasePath,
305
+	if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
306 306
 		fmt.Sprintf("PullRequest.Merge (git fetch): %s", tmpBasePath),
307 307
 		"git", "fetch", "head_repo"); err != nil {
308 308
 		return fmt.Errorf("git fetch [%s -> %s]: %s", headRepoPath, tmpBasePath, stderr)
309 309
 	}
310 310
 
311
-	if _, stderr, err = process.ExecDir(-1, tmpBasePath,
311
+	if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
312 312
 		fmt.Sprintf("PullRequest.Merge (git merge --no-ff --no-commit): %s", tmpBasePath),
313 313
 		"git", "merge", "--no-ff", "--no-commit", "head_repo/"+pr.HeadBranch); err != nil {
314 314
 		return fmt.Errorf("git merge --no-ff --no-commit [%s]: %v - %s", tmpBasePath, err, stderr)
315 315
 	}
316 316
 
317 317
 	sig := doer.NewGitSig()
318
-	if _, stderr, err = process.ExecDir(-1, tmpBasePath,
318
+	if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
319 319
 		fmt.Sprintf("PullRequest.Merge (git merge): %s", tmpBasePath),
320 320
 		"git", "commit", fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email),
321 321
 		"-m", fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.HeadUserName, pr.HeadRepo.Name, pr.BaseBranch)); err != nil {
@@ -323,7 +323,7 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository) (err error
323 323
 	}
324 324
 
325 325
 	// Push back to upstream.
326
-	if _, stderr, err = process.ExecDir(-1, tmpBasePath,
326
+	if _, stderr, err = process.GetManager().ExecDir(-1, tmpBasePath,
327 327
 		fmt.Sprintf("PullRequest.Merge (git push): %s", tmpBasePath),
328 328
 		"git", "push", baseGitRepo.Path, pr.BaseBranch); err != nil {
329 329
 		return fmt.Errorf("git push: %s", stderr)
@@ -437,14 +437,14 @@ func (pr *PullRequest) testPatch() (err error) {
437 437
 	defer os.Remove(indexTmpPath)
438 438
 
439 439
 	var stderr string
440
-	_, stderr, err = process.ExecDirEnv(-1, "", fmt.Sprintf("testPatch (git read-tree): %d", pr.BaseRepo.ID),
440
+	_, stderr, err = process.GetManager().ExecDirEnv(-1, "", fmt.Sprintf("testPatch (git read-tree): %d", pr.BaseRepo.ID),
441 441
 		[]string{"GIT_DIR=" + pr.BaseRepo.RepoPath(), "GIT_INDEX_FILE=" + indexTmpPath},
442 442
 		"git", "read-tree", pr.BaseBranch)
443 443
 	if err != nil {
444 444
 		return fmt.Errorf("git read-tree --index-output=%s %s: %v - %s", indexTmpPath, pr.BaseBranch, err, stderr)
445 445
 	}
446 446
 
447
-	_, stderr, err = process.ExecDirEnv(-1, "", fmt.Sprintf("testPatch (git apply --check): %d", pr.BaseRepo.ID),
447
+	_, stderr, err = process.GetManager().ExecDirEnv(-1, "", fmt.Sprintf("testPatch (git apply --check): %d", pr.BaseRepo.ID),
448 448
 		[]string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()},
449 449
 		"git", "apply", "--check", "--cached", patchPath)
450 450
 	if err != nil {

+ 9 - 11
models/release.go

@@ -10,14 +10,11 @@ import (
10 10
 	"strings"
11 11
 	"time"
12 12
 
13
-	"github.com/go-xorm/xorm"
14
-
15 13
 	"code.gitea.io/git"
16
-
17 14
 	"code.gitea.io/gitea/modules/process"
18 15
 	"code.gitea.io/gitea/modules/setting"
19
-
20 16
 	api "code.gitea.io/sdk/gitea"
17
+	"github.com/go-xorm/xorm"
21 18
 )
22 19
 
23 20
 // Release represents a release of repository.
@@ -159,7 +156,7 @@ func createTag(gitRepo *git.Repository, rel *Release) error {
159 156
 
160 157
 func addReleaseAttachments(releaseID int64, attachmentUUIDs []string) (err error) {
161 158
 	// Check attachments
162
-	var attachments = make([]*Attachment,0)
159
+	var attachments = make([]*Attachment, 0)
163 160
 	for _, uuid := range attachmentUUIDs {
164 161
 		attach, err := getAttachmentByUUID(x, uuid)
165 162
 		if err != nil {
@@ -257,9 +254,10 @@ func GetReleasesByRepoIDAndNames(repoID int64, tagNames []string) (rels []*Relea
257 254
 }
258 255
 
259 256
 type releaseMetaSearch struct {
260
-	ID [] int64
261
-	Rel [] *Release
257
+	ID  []int64
258
+	Rel []*Release
262 259
 }
260
+
263 261
 func (s releaseMetaSearch) Len() int {
264 262
 	return len(s.ID)
265 263
 }
@@ -272,18 +270,18 @@ func (s releaseMetaSearch) Less(i, j int) bool {
272 270
 }
273 271
 
274 272
 // GetReleaseAttachments retrieves the attachments for releases
275
-func GetReleaseAttachments(rels ... *Release) (err error){
273
+func GetReleaseAttachments(rels ...*Release) (err error) {
276 274
 	if len(rels) == 0 {
277 275
 		return
278 276
 	}
279 277
 
280
-	// To keep this efficient as possible sort all releases by id, 
278
+	// To keep this efficient as possible sort all releases by id,
281 279
 	//    select attachments by release id,
282 280
 	//    then merge join them
283 281
 
284 282
 	// Sort
285 283
 	var sortedRels = releaseMetaSearch{ID: make([]int64, len(rels)), Rel: make([]*Release, len(rels))}
286
-	var attachments [] *Attachment
284
+	var attachments []*Attachment
287 285
 	for index, element := range rels {
288 286
 		element.Attachments = []*Attachment{}
289 287
 		sortedRels.ID[index] = element.ID
@@ -375,7 +373,7 @@ func DeleteReleaseByID(id int64, u *User, delTag bool) error {
375 373
 	}
376 374
 
377 375
 	if delTag {
378
-		_, stderr, err := process.ExecDir(-1, repo.RepoPath(),
376
+		_, stderr, err := process.GetManager().ExecDir(-1, repo.RepoPath(),
379 377
 			fmt.Sprintf("DeleteReleaseByID (git tag -d): %d", rel.ID),
380 378
 			"git", "tag", "-d", rel.TagName)
381 379
 		if err != nil && !strings.Contains(stderr, "not found") {

+ 14 - 12
models/repo.go

@@ -147,10 +147,10 @@ func NewRepoContext() {
147 147
 
148 148
 	// Git requires setting user.name and user.email in order to commit changes.
149 149
 	for configKey, defaultValue := range map[string]string{"user.name": "Gitea", "user.email": "gitea@fake.local"} {
150
-		if stdout, stderr, err := process.Exec("NewRepoContext(get setting)", "git", "config", "--get", configKey); err != nil || strings.TrimSpace(stdout) == "" {
150
+		if stdout, stderr, err := process.GetManager().Exec("NewRepoContext(get setting)", "git", "config", "--get", configKey); err != nil || strings.TrimSpace(stdout) == "" {
151 151
 			// ExitError indicates this config is not set
152 152
 			if _, ok := err.(*exec.ExitError); ok || strings.TrimSpace(stdout) == "" {
153
-				if _, stderr, gerr := process.Exec("NewRepoContext(set "+configKey+")", "git", "config", "--global", configKey, defaultValue); gerr != nil {
153
+				if _, stderr, gerr := process.GetManager().Exec("NewRepoContext(set "+configKey+")", "git", "config", "--global", configKey, defaultValue); gerr != nil {
154 154
 					log.Fatal(4, "Fail to set git %s(%s): %s", configKey, gerr, stderr)
155 155
 				}
156 156
 				log.Info("Git config %s set to %s", configKey, defaultValue)
@@ -161,7 +161,7 @@ func NewRepoContext() {
161 161
 	}
162 162
 
163 163
 	// Set git some configurations.
164
-	if _, stderr, err := process.Exec("NewRepoContext(git config --global core.quotepath false)",
164
+	if _, stderr, err := process.GetManager().Exec("NewRepoContext(git config --global core.quotepath false)",
165 165
 		"git", "config", "--global", "core.quotepath", "false"); err != nil {
166 166
 		log.Fatal(4, "Fail to execute 'git config --global core.quotepath false': %s", stderr)
167 167
 	}
@@ -797,20 +797,20 @@ func CleanUpMigrateInfo(repo *Repository) (*Repository, error) {
797 797
 // initRepoCommit temporarily changes with work directory.
798 798
 func initRepoCommit(tmpPath string, sig *git.Signature) (err error) {
799 799
 	var stderr string
800
-	if _, stderr, err = process.ExecDir(-1,
800
+	if _, stderr, err = process.GetManager().ExecDir(-1,
801 801
 		tmpPath, fmt.Sprintf("initRepoCommit (git add): %s", tmpPath),
802 802
 		"git", "add", "--all"); err != nil {
803 803
 		return fmt.Errorf("git add: %s", stderr)
804 804
 	}
805 805
 
806
-	if _, stderr, err = process.ExecDir(-1,
806
+	if _, stderr, err = process.GetManager().ExecDir(-1,
807 807
 		tmpPath, fmt.Sprintf("initRepoCommit (git commit): %s", tmpPath),
808 808
 		"git", "commit", fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email),
809 809
 		"-m", "Initial commit"); err != nil {
810 810
 		return fmt.Errorf("git commit: %s", stderr)
811 811
 	}
812 812
 
813
-	if _, stderr, err = process.ExecDir(-1,
813
+	if _, stderr, err = process.GetManager().ExecDir(-1,
814 814
 		tmpPath, fmt.Sprintf("initRepoCommit (git push): %s", tmpPath),
815 815
 		"git", "push", "origin", "master"); err != nil {
816 816
 		return fmt.Errorf("git push: %s", stderr)
@@ -856,8 +856,10 @@ func getRepoInitFile(tp, name string) ([]byte, error) {
856 856
 
857 857
 func prepareRepoCommit(repo *Repository, tmpDir, repoPath string, opts CreateRepoOptions) error {
858 858
 	// Clone to temporary path and do the init commit.
859
-	_, stderr, err := process.Exec(
860
-		fmt.Sprintf("initRepository(git clone): %s", repoPath), "git", "clone", repoPath, tmpDir)
859
+	_, stderr, err := process.GetManager().Exec(
860
+		fmt.Sprintf("initRepository(git clone): %s", repoPath),
861
+		"git", "clone", repoPath, tmpDir,
862
+	)
861 863
 	if err != nil {
862 864
 		return fmt.Errorf("git clone: %v - %s", err, stderr)
863 865
 	}
@@ -1066,7 +1068,7 @@ func CreateRepository(u *User, opts CreateRepoOptions) (_ *Repository, err error
1066 1068
 			return nil, fmt.Errorf("initRepository: %v", err)
1067 1069
 		}
1068 1070
 
1069
-		_, stderr, err := process.ExecDir(-1,
1071
+		_, stderr, err := process.GetManager().ExecDir(-1,
1070 1072
 			repoPath, fmt.Sprintf("CreateRepository(git update-server-info): %s", repoPath),
1071 1073
 			"git", "update-server-info")
1072 1074
 		if err != nil {
@@ -1839,7 +1841,7 @@ func GitGcRepos() error {
1839 1841
 				if err := repo.GetOwner(); err != nil {
1840 1842
 					return err
1841 1843
 				}
1842
-				_, stderr, err := process.ExecDir(
1844
+				_, stderr, err := process.GetManager().ExecDir(
1843 1845
 					time.Duration(setting.Git.Timeout.GC)*time.Second,
1844 1846
 					RepoPath(repo.Owner.Name, repo.Name), "Repository garbage collection",
1845 1847
 					"git", args...)
@@ -2192,14 +2194,14 @@ func ForkRepository(u *User, oldRepo *Repository, name, desc string) (_ *Reposit
2192 2194
 	}
2193 2195
 
2194 2196
 	repoPath := RepoPath(u.Name, repo.Name)
2195
-	_, stderr, err := process.ExecTimeout(10*time.Minute,
2197
+	_, stderr, err := process.GetManager().ExecTimeout(10*time.Minute,
2196 2198
 		fmt.Sprintf("ForkRepository(git clone): %s/%s", u.Name, repo.Name),
2197 2199
 		"git", "clone", "--bare", oldRepo.RepoPath(), repoPath)
2198 2200
 	if err != nil {
2199 2201
 		return nil, fmt.Errorf("git clone: %v", stderr)
2200 2202
 	}
2201 2203
 
2202
-	_, stderr, err = process.ExecDir(-1,
2204
+	_, stderr, err = process.GetManager().ExecDir(-1,
2203 2205
 		repoPath, fmt.Sprintf("ForkRepository(git update-server-info): %s", repoPath),
2204 2206
 		"git", "update-server-info")
2205 2207
 	if err != nil {

+ 2 - 2
models/repo_editor.go

@@ -212,8 +212,8 @@ func (repo *Repository) GetDiffPreview(branch, treePath, content string) (diff *
212 212
 		return nil, fmt.Errorf("Start: %v", err)
213 213
 	}
214 214
 
215
-	pid := process.Add(fmt.Sprintf("GetDiffPreview [repo_path: %s]", repo.RepoPath()), cmd)
216
-	defer process.Remove(pid)
215
+	pid := process.GetManager().Add(fmt.Sprintf("GetDiffPreview [repo_path: %s]", repo.RepoPath()), cmd)
216
+	defer process.GetManager().Remove(pid)
217 217
 
218 218
 	diff, err = ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdout)
219 219
 	if err != nil {

+ 2 - 2
models/repo_mirror.go

@@ -137,7 +137,7 @@ func (m *Mirror) runSync() bool {
137 137
 		gitArgs = append(gitArgs, "--prune")
138 138
 	}
139 139
 
140
-	if _, stderr, err := process.ExecDir(
140
+	if _, stderr, err := process.GetManager().ExecDir(
141 141
 		timeout, repoPath, fmt.Sprintf("Mirror.runSync: %s", repoPath),
142 142
 		"git", gitArgs...); err != nil {
143 143
 		desc := fmt.Sprintf("Fail to update mirror repository '%s': %s", repoPath, stderr)
@@ -148,7 +148,7 @@ func (m *Mirror) runSync() bool {
148 148
 		return false
149 149
 	}
150 150
 	if m.Repo.HasWiki() {
151
-		if _, stderr, err := process.ExecDir(
151
+		if _, stderr, err := process.GetManager().ExecDir(
152 152
 			timeout, wikiPath, fmt.Sprintf("Mirror.runSync: %s", wikiPath),
153 153
 			"git", "remote", "update", "--prune"); err != nil {
154 154
 			desc := fmt.Sprintf("Fail to update mirror wiki repository '%s': %s", wikiPath, stderr)

+ 2 - 2
models/ssh_key.go

@@ -197,7 +197,7 @@ func SSHKeyGenParsePublicKey(key string) (string, int, error) {
197 197
 	}
198 198
 	defer os.Remove(tmpName)
199 199
 
200
-	stdout, stderr, err := process.Exec("SSHKeyGenParsePublicKey", setting.SSH.KeygenPath, "-lf", tmpName)
200
+	stdout, stderr, err := process.GetManager().Exec("SSHKeyGenParsePublicKey", setting.SSH.KeygenPath, "-lf", tmpName)
201 201
 	if err != nil {
202 202
 		return "", 0, fmt.Errorf("fail to parse public key: %s - %s", err, stderr)
203 203
 	}
@@ -382,7 +382,7 @@ func addKey(e Engine, key *PublicKey) (err error) {
382 382
 	if err = ioutil.WriteFile(tmpPath, []byte(key.Content), 0644); err != nil {
383 383
 		return err
384 384
 	}
385
-	stdout, stderr, err := process.Exec("AddPublicKey", "ssh-keygen", "-lf", tmpPath)
385
+	stdout, stderr, err := process.GetManager().Exec("AddPublicKey", "ssh-keygen", "-lf", tmpPath)
386 386
 	if err != nil {
387 387
 		return fmt.Errorf("'ssh-keygen -lf %s' failed with error '%s': %s", tmpPath, err, stderr)
388 388
 	} else if len(stdout) < 2 {

+ 87 - 72
modules/process/manager.go

@@ -9,71 +9,108 @@ import (
9 9
 	"errors"
10 10
 	"fmt"
11 11
 	"os/exec"
12
+	"sync"
12 13
 	"time"
13 14
 
14 15
 	"code.gitea.io/gitea/modules/log"
15 16
 )
16 17
 
18
+// TODO: This packages still uses a singleton for the Manager.
19
+// Once there's a decent web framework and dependencies are passed around like they should,
20
+// then we delete the singleton.
21
+
17 22
 var (
18 23
 	// ErrExecTimeout represent a timeout error
19 24
 	ErrExecTimeout = errors.New("Process execution timeout")
20
-
21
-	// DefaultTimeout is the timeout used by Exec* family
22
-	// of function when timeout parameter is omitted or
23
-	// passed as -1
24
-	// NOTE: could be custom in config file for default.
25
-	DefaultTimeout = 60 * time.Second
25
+	manager        *Manager
26 26
 )
27 27
 
28 28
 // Process represents a working process inherit from Gogs.
29 29
 type Process struct {
30
-	Pid         int64 // Process ID, not system one.
30
+	PID         int64 // Process ID, not system one.
31 31
 	Description string
32 32
 	Start       time.Time
33 33
 	Cmd         *exec.Cmd
34 34
 }
35 35
 
36
-// List of existing processes.
37
-var (
38
-	curPid    int64 = 1
39
-	Processes []*Process
40
-)
36
+// Manager knows about all processes and counts PIDs.
37
+type Manager struct {
38
+	mutex sync.Mutex
41 39
 
42
-// Add adds a existing process and returns its PID.
43
-func Add(desc string, cmd *exec.Cmd) int64 {
44
-	pid := curPid
45
-	Processes = append(Processes, &Process{
46
-		Pid:         pid,
47
-		Description: desc,
40
+	counter   int64
41
+	Processes map[int64]*Process
42
+}
43
+
44
+// GetManager returns a Manager and initializes one as singleton if there's none yet
45
+func GetManager() *Manager {
46
+	if manager == nil {
47
+		manager = &Manager{
48
+			Processes: make(map[int64]*Process),
49
+		}
50
+	}
51
+	return manager
52
+}
53
+
54
+// Add a process to the ProcessManager and returns its PID.
55
+func (pm *Manager) Add(description string, cmd *exec.Cmd) int64 {
56
+	pm.mutex.Lock()
57
+	pid := pm.counter + 1
58
+	pm.Processes[pid] = &Process{
59
+		PID:         pid,
60
+		Description: description,
48 61
 		Start:       time.Now(),
49 62
 		Cmd:         cmd,
50
-	})
51
-	curPid++
63
+	}
64
+	pm.counter = pid
65
+	pm.mutex.Unlock()
66
+
52 67
 	return pid
53 68
 }
54 69
 
70
+// Remove a process from the ProcessManager.
71
+func (pm *Manager) Remove(pid int64) {
72
+	pm.mutex.Lock()
73
+	delete(pm.Processes, pid)
74
+	pm.mutex.Unlock()
75
+}
76
+
77
+// Exec a command and use the default timeout.
78
+func (pm *Manager) Exec(desc, cmdName string, args ...string) (string, string, error) {
79
+	return pm.ExecDir(-1, "", desc, cmdName, args...)
80
+}
81
+
82
+// ExecTimeout a command and use a specific timeout duration.
83
+func (pm *Manager) ExecTimeout(timeout time.Duration, desc, cmdName string, args ...string) (string, string, error) {
84
+	return pm.ExecDir(timeout, "", desc, cmdName, args...)
85
+}
86
+
87
+// ExecDir a command and use the default timeout.
88
+func (pm *Manager) ExecDir(timeout time.Duration, dir, desc, cmdName string, args ...string) (string, string, error) {
89
+	return pm.ExecDirEnv(timeout, dir, desc, nil, cmdName, args...)
90
+}
91
+
55 92
 // ExecDirEnv runs a command in given path and environment variables, and waits for its completion
56 93
 // up to the given timeout (or DefaultTimeout if -1 is given).
57 94
 // Returns its complete stdout and stderr
58 95
 // outputs and an error, if any (including timeout)
59
-func ExecDirEnv(timeout time.Duration, dir, desc string, env []string, cmdName string, args ...string) (string, string, error) {
96
+func (pm *Manager) ExecDirEnv(timeout time.Duration, dir, desc string, env []string, cmdName string, args ...string) (string, string, error) {
60 97
 	if timeout == -1 {
61
-		timeout = DefaultTimeout
98
+		timeout = 60 * time.Second
62 99
 	}
63 100
 
64
-	bufOut := new(bytes.Buffer)
65
-	bufErr := new(bytes.Buffer)
101
+	stdOut := new(bytes.Buffer)
102
+	stdErr := new(bytes.Buffer)
66 103
 
67 104
 	cmd := exec.Command(cmdName, args...)
68 105
 	cmd.Dir = dir
69 106
 	cmd.Env = env
70
-	cmd.Stdout = bufOut
71
-	cmd.Stderr = bufErr
107
+	cmd.Stdout = stdOut
108
+	cmd.Stderr = stdErr
72 109
 	if err := cmd.Start(); err != nil {
73
-		return "", err.Error(), err
110
+		return "", "", err
74 111
 	}
75 112
 
76
-	pid := Add(desc, cmd)
113
+	pid := pm.Add(desc, cmd)
77 114
 	done := make(chan error)
78 115
 	go func() {
79 116
 		done <- cmd.Wait()
@@ -82,61 +119,39 @@ func ExecDirEnv(timeout time.Duration, dir, desc string, env []string, cmdName s
82 119
 	var err error
83 120
 	select {
84 121
 	case <-time.After(timeout):
85
-		if errKill := Kill(pid); errKill != nil {
86
-			log.Error(4, "Exec(%d:%s): %v", pid, desc, errKill)
122
+		if errKill := pm.Kill(pid); errKill != nil {
123
+			log.Error(4, "exec(%d:%s) failed to kill: %v", pid, desc, errKill)
87 124
 		}
88 125
 		<-done
89
-		return "", ErrExecTimeout.Error(), ErrExecTimeout
126
+		return "", "", ErrExecTimeout
90 127
 	case err = <-done:
91 128
 	}
92 129
 
93
-	Remove(pid)
94
-	return bufOut.String(), bufErr.String(), err
95
-}
130
+	pm.Remove(pid)
96 131
 
97
-// ExecDir works exactly like ExecDirEnv except no environment variable is provided.
98
-func ExecDir(timeout time.Duration, dir, desc, cmdName string, args ...string) (string, string, error) {
99
-	return ExecDirEnv(timeout, dir, desc, nil, cmdName, args...)
100
-}
101
-
102
-// ExecTimeout runs a command and waits for its completion
103
-// up to the given timeout (or DefaultTimeout if -1 is given).
104
-// Returns its complete stdout and stderr
105
-// outputs and an error, if any (including timeout)
106
-func ExecTimeout(timeout time.Duration, desc, cmdName string, args ...string) (string, string, error) {
107
-	return ExecDir(timeout, "", desc, cmdName, args...)
108
-}
109
-
110
-// Exec runs a command and waits for its completion
111
-// up to DefaultTimeout. Returns its complete stdout and stderr
112
-// outputs and an error, if any (including timeout)
113
-func Exec(desc, cmdName string, args ...string) (string, string, error) {
114
-	return ExecDir(-1, "", desc, cmdName, args...)
115
-}
116
-
117
-// Remove removes a process from list.
118
-func Remove(pid int64) {
119
-	for i, proc := range Processes {
120
-		if proc.Pid == pid {
121
-			Processes = append(Processes[:i], Processes[i+1:]...)
122
-			return
123
-		}
132
+	if err != nil {
133
+		out := fmt.Errorf("exec(%d:%s) failed: %v stdout: %v stderr: %v", pid, desc, err, stdOut, stdErr)
134
+		return stdOut.String(), stdErr.String(), out
124 135
 	}
136
+
137
+	return stdOut.String(), stdErr.String(), nil
125 138
 }
126 139
 
127
-// Kill kills and removes a process from list.
128
-func Kill(pid int64) error {
129
-	for i, proc := range Processes {
130
-		if proc.Pid == pid {
131
-			if proc.Cmd != nil && proc.Cmd.Process != nil &&
132
-				proc.Cmd.ProcessState != nil && !proc.Cmd.ProcessState.Exited() {
133
-				if err := proc.Cmd.Process.Kill(); err != nil {
134
-					return fmt.Errorf("fail to kill process(%d/%s): %v", proc.Pid, proc.Description, err)
135
-				}
140
+// Kill and remove a process from list.
141
+func (pm *Manager) Kill(pid int64) error {
142
+	if proc, exists := pm.Processes[pid]; exists {
143
+		pm.mutex.Lock()
144
+		if proc.Cmd != nil &&
145
+			proc.Cmd.Process != nil &&
146
+			proc.Cmd.ProcessState != nil &&
147
+			!proc.Cmd.ProcessState.Exited() {
148
+			if err := proc.Cmd.Process.Kill(); err != nil {
149
+				return fmt.Errorf("failed to kill process(%d/%s): %v", pid, proc.Description, err)
136 150
 			}
137
-			Processes = append(Processes[:i], Processes[i+1:]...)
138
-			return nil
139 151
 		}
152
+		delete(pm.Processes, pid)
153
+		pm.mutex.Unlock()
140 154
 	}
155
+
141 156
 	return nil
142 157
 }

+ 33 - 0
modules/process/manager_test.go

@@ -0,0 +1,33 @@
1
+package process
2
+
3
+import (
4
+	"os/exec"
5
+	"testing"
6
+
7
+	"github.com/stretchr/testify/assert"
8
+)
9
+
10
+func TestManager_Add(t *testing.T) {
11
+	pm := Manager{Processes: make(map[int64]*Process)}
12
+
13
+	pid := pm.Add("foo", exec.Command("foo"))
14
+	assert.Equal(t, int64(1), pid, "expected to get pid 1 got %d", pid)
15
+
16
+	pid = pm.Add("bar", exec.Command("bar"))
17
+	assert.Equal(t, int64(2), pid, "expected to get pid 2 got %d", pid)
18
+}
19
+
20
+func TestManager_Remove(t *testing.T) {
21
+	pm := Manager{Processes: make(map[int64]*Process)}
22
+
23
+	pid1 := pm.Add("foo", exec.Command("foo"))
24
+	assert.Equal(t, int64(1), pid1, "expected to get pid 1 got %d", pid1)
25
+
26
+	pid2 := pm.Add("bar", exec.Command("bar"))
27
+	assert.Equal(t, int64(2), pid2, "expected to get pid 2 got %d", pid2)
28
+
29
+	pm.Remove(pid2)
30
+
31
+	_, exists := pm.Processes[pid2]
32
+	assert.False(t, exists, "PID %d is in the list but shouldn't", pid2)
33
+}

+ 1 - 1
routers/admin/admin.go

@@ -246,7 +246,7 @@ func Monitor(ctx *context.Context) {
246 246
 	ctx.Data["Title"] = ctx.Tr("admin.monitor")
247 247
 	ctx.Data["PageIsAdmin"] = true
248 248
 	ctx.Data["PageIsAdminMonitor"] = true
249
-	ctx.Data["Processes"] = process.Processes
249
+	ctx.Data["Processes"] = process.GetManager().Processes
250 250
 	ctx.Data["Entries"] = cron.ListTasks()
251 251
 	ctx.HTML(200, tplMonitor)
252 252
 }