I've noticed that after a couple minutes, this resolves itself. Why? Is there something we could be doing after the command is interrupted (in our http.CloseNotifier-receiving code) to more aggressive kill processes or delete things?

When the HTTP connection hangs up, the buildlet calls cmd.Process.Kill()
but the process may not have quit by the time you issued the next "push".

Hard to say what we could do beyond this. Maybe do the cmd.Wait in a
separate goroutine and block future "pushes" until the process actually
exits? Seems a bit gross.

On 6 May 2015 at 07:07, Brad Fitzpatrick notifications@github.com wrote:

I've noticed that after a couple minutes, this resolves itself. Why? Is
there something we could be doing after the command is interrupted (in our
http.CloseNotifier-receiving code) to more aggressive kill processes or
delete things?

—
Reply to this email directly or view it on GitHub
#10706 (comment).

I am trying to debug this, so I followed steps in golang.org/x/build/cmd/buildlet/README and I got stuck on:

# curl -k --user :foo -d "cmd=src/make.bat" https://alexpc:5936
/exec                                                                                
##### Building C bootstrap tool.                                                     
cmd/dist                                                                             
go tool dist: FAILED: git rev-parse --abbrev-ref HEAD                                

The repo from https://go.googlesource.com/go/+archive/3b76b017cabb.tar.gz does not have .git directory. What do I need to do to get past this message?

As to the reason of

remove C:\workdir\go\src\cmd\dist\dist.exe: Access is denied.

(just a guess) you kill process when connection is disconnected, but I suspect you don't disconnect properly. Does killing gomote process clears remote side of its TCP connection? Maybe it takes a while. You also don't check cmd.Process.Kill() returned error, perhaps, if you add more error checking / printing, we'll know more.

But if you help me with error above maybe I can reproduce it here. Thank you.

Alex

The coordinator and gomote binaries push a VERSION file to the buildlets first, so git isn't required to build.

The gomote binary accepts a "host:port" string as a name, to use it against localhost.

Thank you Brad. I caught him (C:\workdir\go\src\cmd\dist\dist.exe) in the act. More details coming.

Alex

Exciting. :)

You have used your jokes quota today. Please stop.

Alex

No, I'm actually excited. I love a good debugging mystery, and clues!

Thanks for investigating this.

I can see make.bat process gets killed, but none of its children are. cmd/dist.exe process runs to completion even after gomote is disconnected. That is why you saw "remove C:\workdir\go\src\cmd\dist\dist.exe: Access is denied." message, because you tried to rebuild executable for process that is still running - this is not allowed on Windows. But then cmd/dist.exe completes (it takes minute or so), and you can rebuild it then.

Our mistake. We all (mainly me) assumed that killing process kills all its children. But that is not true on Windows. Not by default. I can come up with 2 ways to implement process tree killing.

We can enumerate all processes on the system and kill all children of a process. See syscall.Getppid for something similar.

Or we can use "recommended" way - associate our process with a job object https://msdn.microsoft.com/en-us/library/ms684161(VS.85).aspx We already used job objects before to gather performance stats on windows (golang.org/x/benchmarks/driver/driver_windows.go).

I don't know how important it is to kill all children here in buildlet. All these computers are rebooted often anyway. Brad?

Alex

I want to fix this. The purpose of gomote is interactive development. If you can't stop a build and restart it quickly, it's very annoying.

Rather than change Go for now, we can just change the buildlet to enumerate all the processes on the children.

Thanks for the tips and debugging.

https://go-review.googlesource.com/9837

CL https://golang.org/cl/9837 mentions this issue.