Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/dist: child process prints to log after make.bash has exited #25981

Closed
alandonovan opened this issue Jun 20, 2018 · 6 comments
Closed

cmd/dist: child process prints to log after make.bash has exited #25981

alandonovan opened this issue Jun 20, 2018 · 6 comments

Comments

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Jun 20, 2018

$ cd goroot/src
$ git reset --hard dd0e7a9 # pristine recent snapshot
$ echo  "doesn't compile" >> cmd/go/internal/list/list.go
$ ./make.bash
Building Go cmd/dist using /usr/local/google/home/adonovan/go1.6.
Building Go toolchain1 using /usr/local/google/home/adonovan/go1.6.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
/usr/local/google/home/adonovan/goroot2/src/cmd/go/internal/list/list.go:421:1: syntax error: non-declaration statement outside function body
/usr/local/google/home/adonovan/goroot2/src/cmd/go/internal/list/list.go:421:16: newline in character literal
go tool dist: FAILED: /usr/local/google/home/adonovan/goroot2/pkg/tool/linux_amd64/compile -std -pack -o /tmp/go-tool-dist-385632282/cmd/go/internal/list/_go_.a -p cmd/go/internal/list /usr/local/google/home/adonovan/goroot2/src/cmd/go/internal/list/context.go /usr/local/google/home/adonovan/goroot2/src/cmd/go/internal/list/list.go: exit status 2
$ can't create /tmp/go-tool-dist-385632282/cmd/go/internal/run/_go_.a: open /tmp/go-tool-dist-385632282/cmd/go/internal/run/_go_.a: no such file or directory
can't create /tmp/go-tool-dist-385632282/cmd/go/internal/vet/_go_.a: open /tmp/go-tool-dist-385632282/cmd/go/internal/vet/_go_.a: no such file or directory
...

Notice the $ sign appearing in the middle of the output. This indicates that make.bash exited and the shell printed its prompt, but some child process of make.bash is still printing log output. make.bash should wait for its children to exit before exiting itself.

@ianlancetaylor ianlancetaylor changed the title make.bash: child process prints to log after make.bash has exited cmd/dist: child process prints to log after make.bash has exited Jun 20, 2018
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 20, 2018

The error is most likely coming from the compiler. I'm pretty sure it's because the call to run in runInstall in cmd/dist/build.go will exit the dist tool if the compilation fails, without waiting for any other compilations running in parallel.

@LotusFenn
Copy link
Contributor

@LotusFenn LotusFenn commented Jul 8, 2018

I'm going to work on this issue.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Aug 3, 2018

@ianlancetaylor - Looked into this. The run actually waits for background processes to finish when fatalf gets called. But it seems there are additional foreground run processes running which need to be waited for.

If I add a simple sync.WaitGroup for every run call and .Wait when fatalf is called, the issue is resolved. But I am wondering if this is the right approach ? We will how have 2 waitgroups - one is already bgHelpers, now we will have fgHelpers. Should we somehow combine all of these into a single waitgroup or look at some alternate solution ?

Or maybe I missed something altogether, please do let me know if that is the case.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 3, 2018

Hmmm, this does seem kind of complicated as it seems easy to get into a deadlock situation, in which fatalf is waiting for its own goroutine to complete. The current code avoids that by having run call bghelpers.Done before calling fatalf, but adding more complexity here will just make things even harer to understand.

In some sense code that is running in a goroutine, like runInstall, ought not to call run. I suggest that we change the two calls to run in runInstall to call bgrun instead, with a new WaitGroup, and immediately wait for that WaitGroup. That will push the calls into the existing mechanism.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Aug 3, 2018

Cool. We don't even need a new WaitGroup. Since we are immediately waiting, I just moved the same waitgroup above and it worked fine.

Will send a CL.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 3, 2018

Change https://golang.org/cl/127776 mentions this issue: cmd/dist: wait for run jobs to finish in case of a compiler error

@gopherbot gopherbot closed this in 04bee23 Aug 30, 2018
@golang golang locked and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.