Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
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
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.12

Comments

@alandonovan
Copy link
Contributor

alandonovan commented Jun 20, 2018 •
edited by josharian
Loading 

$ 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
Member

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.

@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Jun 20, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jun 20, 2018
@LotusFenn
Copy link
Contributor

I'm going to work on this issue.

@agnivade
Copy link
Contributor

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
Member

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 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
Contributor

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

@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.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
5 participants
@agnivade @ianlancetaylor @alandonovan @gopherbot @LotusFenn