Fix run command race #1470

typeless · 2017-04-08T07:56:42Z

Use exec.CommandContext to simplify timeout handling, and fixing the data races which can be identified by the added tests when -race enabled.

Related to #1453

typeless · 2017-04-08T08:47:35Z

I'm afraid that the stdin is redirected to /dev/null within drone :(
I would have to think about other approach.

bkcsoft · 2017-04-18T06:04:34Z

modules/process/manager.go

@@ -6,13 +6,12 @@ package process

 import (
 	"bytes"
+	"context"


This is only availably in 1.7+, we still need to support 1.6

Why we need to support 1.6?

Because some developers (@strk for example) still run 1.6. Latest version in Ubuntu 16.04 LTS is 1.6 as well http://packages.ubuntu.com/xenial/golang-go

#1349 discussed before.

Still not liking it 😂

We can vendor the package but the reason is less compelling since it doesn't affect end-users. Otherwise we can postpone the PR until Ubuntu catching up with the latest.

By the way, what keeps you (@bkcsoft @strk) using Ubuntu's Go rather than the official one? I'm curious if there's anything I could help.

Next LTS is 18.04, which should include 1.7+. IMO we should wait for that until droping support for 1.6

Personally I've upgraded already, so don't think about me. Just think about random possible contributor staying safe in her latest debian/ubuntu/fedora stable system. We want to reduce the barriers of entry for any contributor.

We should also have Drone test all Go versions we want to support. For comparison Gogs is Travis-tested against Go versions from 1.5 to master.

I don't agree that strictly supporting old toolchains would reward the project's developers and users that much, if any. Just speaking for my own opinion though. ❤️

18.10 is released and have Go 1.8 so no need for this change.

strk · 2017-04-20T10:40:04Z

It's not about reward but entry barrier. Developers are a scarce resource, and even many of the documented maintainers are being increasily inactive. For these reasons I think we should lower the barrier as much as possible. From low dependencies to documented procedures and codebase, to easy to use testing frameworks...

lunny · 2017-05-25T03:04:42Z

We could drop Go v1.6 support on Gitea v1.3, so I moved this to v1.3

sapk · 2017-05-26T14:50:29Z

Go v1.6 is already drop in git module so dropped in gitea. https://github.com/go-gitea/git/blob/master/command.go#L9

lunny · 2017-05-26T15:03:01Z

So that we only need to add an implicated declare on some README or other document.

lafriks · 2017-10-25T20:55:55Z

@bkcsoft need your approval

bkcsoft · 2017-10-29T06:16:45Z

modules/process/manager.go

-	if err != nil {
-		out := fmt.Errorf("exec(%d:%s) failed: %v stdout: %v stderr: %v", pid, desc, err, stdOut, stdErr)
-		return stdOut.String(), stdErr.String(), out
+	if err == nil {


I'd change the error-handling to a more go-esque way

if err != nil { err = fmt.Errorf(...) // NOTE: include the original error as well as the `ctx.Err()`... } return stdOut.String(), stdErr.String(), err

@typeless maybe you could change this.

bkcsoft · 2017-10-29T06:21:25Z

modules/process/manager.go

@@ -101,7 +100,10 @@ func (pm *Manager) ExecDirEnv(timeout time.Duration, dir, desc string, env []str
 	stdOut := new(bytes.Buffer)
 	stdErr := new(bytes.Buffer)

-	cmd := exec.Command(cmdName, args...)
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)


What happens if the timeout is negative? (Since the functions docs says to use -1 for DefaultTimeout (which we don't seems to honour... )

@bkcsoft The first line on this function has checked that. if timeout == -1 timout = 60 *time.Second

Do you want

if timeout < 0 { timeout = 60 * time.Second }

?

lunny · 2017-11-07T05:42:29Z

LGTM except @bkcsoft 's second comment I agree with.

bkcsoft · 2017-11-13T06:28:23Z

LGTM

And fixing the data races which can be identified by the added tests when -race enabled.

codecov-io · 2017-11-13T08:58:48Z

Codecov Report

Merging #1470 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #1470     +/-   ##
=========================================
+ Coverage   27.05%   27.26%   +0.2%     
=========================================
  Files          89       89             
  Lines       17650    17640     -10     
=========================================
+ Hits         4775     4809     +34     
+ Misses      12189    12144     -45     
- Partials      686      687      +1

Impacted Files	Coverage Δ
modules/process/manager.go	`73.91% <100%> (+52.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9728bf...41a01f0. Read the comment docs.

typeless · 2017-11-13T10:46:07Z

The following error doesn't seem to be caused by the pull request.

fatal: unable to access 'https://go.googlesource.com/tools/': Failed to connect to go.googlesource.com port 443: Operation timed out
128s

lafriks · 2017-11-13T10:47:56Z

Yes it's just random drone failure

lunny · 2017-11-13T12:37:18Z

@lafriks that's a network error not caused by this PR. I will test it twice again then I will move it to v1.3.0 and merge it.

typeless changed the title ~~Fix run command race~~ [WIP] Fix run command race Apr 8, 2017

typeless force-pushed the fix-run-command-race branch from 488adc3 to 0adf9d4 Compare April 8, 2017 08:02

typeless changed the title ~~[WIP] Fix run command race~~ Fix run command race Apr 8, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 8, 2017

lunny added this to the 1.2.0 milestone Apr 8, 2017

lunny added the type/bug label Apr 8, 2017

bkcsoft suggested changes Apr 18, 2017

View reviewed changes

typeless mentioned this pull request Apr 20, 2017

handleCreateError()] [E] CreatePost: database table is locked: repository' #1398

Closed

7 tasks

lunny modified the milestones: 1.3.0, 1.2.0 May 25, 2017

cez81 mentioned this pull request Jun 14, 2017

Add note Go 1.7 is required for building from source go-gitea/docs#136

Merged

bkcsoft approved these changes Oct 29, 2017

View reviewed changes

bkcsoft suggested changes Oct 29, 2017

View reviewed changes

bkcsoft reviewed Oct 29, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 7, 2017

lunny modified the milestones: 1.3.0, 1.4.0 Nov 10, 2017

bkcsoft approved these changes Nov 13, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 13, 2017

Use exec.CommandContext to simplfy timeout handling

77625c5

And fixing the data races which can be identified by the added tests when -race enabled.

Use sleep commmand instead of reading from stdin

cb66d5b

typeless force-pushed the fix-run-command-race branch from d2400ee to b6be1ff Compare November 13, 2017 07:37

Make the error handling go-esque

7dbf2d2

typeless force-pushed the fix-run-command-race branch from b6be1ff to 7dbf2d2 Compare November 13, 2017 08:44

Merge branch 'master' into fix-run-command-race

0f2a90c

Merge branch 'master' into fix-run-command-race

41a01f0

lunny modified the milestones: 1.4.0, 1.3.0 Nov 13, 2017

lunny merged commit f4d12f8 into go-gitea:master Nov 13, 2017

lunny mentioned this pull request Nov 13, 2017

Data race detected #1453

Closed

7 tasks

typeless deleted the fix-run-command-race branch April 3, 2019 05:01

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix run command race #1470

Fix run command race #1470

typeless commented Apr 8, 2017 •

edited

Loading

typeless commented Apr 8, 2017

bkcsoft Apr 18, 2017

lunny Apr 18, 2017

bkcsoft Apr 18, 2017

appleboy Apr 18, 2017

bkcsoft Apr 18, 2017

typeless Apr 18, 2017

bkcsoft Apr 18, 2017

strk Apr 18, 2017

typeless Apr 20, 2017

bkcsoft Oct 29, 2017

strk commented Apr 20, 2017 via email

lunny commented May 25, 2017

sapk commented May 26, 2017

lunny commented May 26, 2017

lafriks commented Oct 25, 2017

bkcsoft Oct 29, 2017

lunny Nov 5, 2017

typeless Nov 13, 2017

bkcsoft Oct 29, 2017

lunny Nov 1, 2017

typeless Nov 13, 2017

lunny commented Nov 7, 2017

bkcsoft commented Nov 13, 2017

codecov-io commented Nov 13, 2017 •

edited

Loading

typeless commented Nov 13, 2017

lafriks commented Nov 13, 2017

lunny commented Nov 13, 2017

Fix run command race #1470

Fix run command race #1470

Conversation

typeless commented Apr 8, 2017 • edited Loading

typeless commented Apr 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Apr 20, 2017 via email

lunny commented May 25, 2017

sapk commented May 26, 2017

lunny commented May 26, 2017

lafriks commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Nov 7, 2017

bkcsoft commented Nov 13, 2017

codecov-io commented Nov 13, 2017 • edited Loading

Codecov Report

typeless commented Nov 13, 2017

lafriks commented Nov 13, 2017

lunny commented Nov 13, 2017

typeless commented Apr 8, 2017 •

edited

Loading

codecov-io commented Nov 13, 2017 •

edited

Loading