Add process-level buildroot validation to NailgunExecutor.

Review Request #3393 - Created Jan. 29, 2016 and submitted

Information
Kris Wilson
pants
2659, 2855
Reviewers
pants-reviews
jsirois, nhoward_tw, patricklaw, stuhood

In the case of stale pids for nailgun processes, when a given pid is checked for liveness and the new process is also a java process it can pass the process executable name check (java == java) but not actually be a nailgun process owned by the buildroot. Because the NailgunExecutor fingerprint also does not match a given new fingerprint (None != $NEW_SHA), the current logic treats this as a "I am running but have a new fingerprint and thus need to restart" case and attempts to kill the stale pid before starting a new nailgun. This can cause unintended behavior such as permissions related exceptions as seen in #2659 or worse, the killing of unrelated java processes.

This change addresses this case by adding an inline check for a matching nailgun buildroot argument in the NailgunExecutor.is_alive() check.

Fixes #2659.

CI is green @ https://travis-ci.org/pantsbuild/pants/builds/105578526

Additional manual testing:

While monitoring with ps:

1) `./pants clean-all && ./pants compile testprojects/src/java::` in `~/dev/pants` which starts nailgun owned by `~/dev/pants`.
2) `cp -avf ~/dev/pants ~/dev/pants2 && rm -rf ~/dev/pants2/.pants.d` to wipe cache without removing `.pids` (creating a stale pid situation)
3) `cd ~/dev/pants2 && ./pants compile testprojects/src/java::`

Verified that new nailgun processes are launched for the second buildroot and the first buildroot's processes are untouched.

Issues

  • 0
  • 1
  • 1
  • 2
Description From Last Updated
Nick Howard (Twitter)
Patrick Lawson
Kris Wilson
Nick Howard (Twitter)
Kris Wilson
Review request changed

Status: Closed (submitted)

Change Summary:

thanks Nick & Patrick! this is in @ f9c03075bcf5bebeb9bea1d21d2489d2b4c3b9f0

Loading...