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

"saving process ryan", need option to disable killing processes; FAKE kills other FAKEs and also other processes that don't belong to it directly #357

Closed
silky opened this issue Mar 13, 2014 · 11 comments

Comments

@silky
Copy link

silky commented Mar 13, 2014

can we get an option to stop this? or have the killing process ensure that it is the parent, before it kills?

observed on fake ver 2.2.22.0 (and in the current source code)

@forki
Copy link
Member

forki commented Mar 13, 2014

I'd like to see a solution that doesn't need an option. Any ideas how to fix it?

@mexx
Copy link
Member

mexx commented Mar 13, 2014

Currently the started processes are tracked by processId and name, but why not use the Process object? It tracks the status of the spawned process and can kill only that one.
Maybe this would do the trick?

@forki
Copy link
Member

forki commented Mar 13, 2014

We can try this. ==> up-for-grabs

@silky
Copy link
Author

silky commented Mar 13, 2014

the killing is imo verging on completely inappropriate.

fake shouldn't assume it has ultimate authority to kill the processes it starts. it absolutely should be an optional thing for it to do.

the result of this implementation was that our build system, which often runs ~4 builds simultaneously, exploding in a bloodbath of FAKE just killing all the other builds, whenever one finished. it was remarkably unpleasant.

if this isn't made optional, we'll just simply NOT use the helper code from fake to launch processes.

@forki
Copy link
Member

forki commented Mar 13, 2014

mhm.

Making this optional is easy. I will do this, but your description sounds like a bug. A FAKE build should not kill processes of another FAKE build.

@forki
Copy link
Member

forki commented Mar 13, 2014

This small change allows you to write

ProcessHelper.killCreatedProcesses <- false 

at the beginning of your build script.

Is this good enough or do you prefer a command line param for FAKE?

@silky
Copy link
Author

silky commented Mar 13, 2014

thanks for that @forki; that will be perfect for us.

@silky
Copy link
Author

silky commented Mar 13, 2014

we think at the moment that the reason FAKE was killing other processes was due to process id reuse (many many processes are started during our build), as I was unable to reproduce it with small examples (hence the reason I think checking owernship might help, or something like that.)

@forki
Copy link
Member

forki commented Mar 13, 2014

ok the prerelease and release builds are running.

But of course I'd like to see a fix for the root cause.

@forki
Copy link
Member

forki commented Mar 13, 2014

Btw: this feature was created due to very big problems on teamcity CI servers. Under some conditions Nunit.exe remains as a zombie process and slows the teamcity process down. After a couple of builds the whole system freezes.
So we thought it might be a good idea if FAKE checks if everything is in a good state after a build.
I apologize for the trouble with the bad implementation, but I think the idea of the feature is important.

@silky
Copy link
Author

silky commented Mar 13, 2014

no worries forki.

fwiw, this is the exerpt from the build log about what FAKE was doing:

Killing all processes that are created by FAKE and are still running.
Trying to kill process FAKE (Id = 6688)

immediately after this, another build died. all the builds are run independently (through jenkins) so i don't see how FAKE could have decided to kill this process without it either being:

  1. process # re-use,
  2. ???

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants