Revert #544 so that RealJenkinsRule shuts down cleanly on Windows

[dwnusbaum]

Based on CI build failures in jenkinsci/workflow-cps-plugin#669 (see for example this test output), I am suspicious that beyond the lack of shutdown-related log messages on older versions of Jenkins, #544 might not shut down Jenkins cleanly on Windows, in particular I think Jenkins.cleanUp might not run and terminators may be skipped.

This PR injects an ItemListener dynamically into Jenkins using ExtensionList.add that writes a file in onBeforeShutdown, which is called by Jenkins.cleanUp, and after the restart we check for the existence of this file. Maybe there is something else we can already observe without needing this, but without LoggerRule I couldn't think of anything obvious.

RealJenkinsRuleTest.restart passes for me locally on MacOS with this PR.

EDIT: The tests confirmed my suspicions, so this PR now reverts #544 so that RealJenkinsRule shuts down cleanly on Windows.

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[dwnusbaum]

Hmm the CI build does fail on Windows but not on Linux. @jglick was #544 necessary to fix an issue in some scenario, or was it just a simplification that could be reverted? As far as I can tell, that PR causes RealJenkinsRule.stopJenkins to not stop Jenkins cleanly on Windows, even when running Jenkins 2.391+.

[jglick]

It could be reverted I think, but I would like to know why stopping the process does not cause terminators to run—if true, that certainly sounds like a bug?

[dwnusbaum]

I would like to know why stopping the process does not cause terminators to run—if true, that certainly sounds like a bug?

Discussion in JDK-4485742 and linked tickets indicates that Process.destroy terminates the process in a way that bypasses Java shutdown hooks on Windows (which is what Winstone uses to trigger cleanup), and that this will not be changed due to backwards compatibility implications. Looking at JDK source code, ProcessHandle.destroy seems to use the same native Windows API calls (compare this to this), so I assume it has the same behavior.

EDIT: Also, as far as I can tell, Process.supportsNormalTermination() and ProcessHandle.supportsNormalTermination() both return false on Windows.

[jglick]

Also JDK-8056139. Hmm, that is unfortunate. We could fully revert #544, or continue to send SIGTERM on Linux and just use the special /exit endpoint on Windows.

[dwnusbaum]

Is there any reason to prefer ProcessHandle.destroy in cases where ProcessHandle.supportsNormalTermination() returns true? If not, I guess just fully reverting #544 is simplest so that there is only one code path.

[jglick]

any reason to prefer ProcessHandle.destroy

Merely to avoid a gratuitous use of a different code path than what would often be used in a realistic context. I agree that having two modes is more confusing that it would be worth, so reverting #544 would be the simplest fix. Feel free to file that (it sounds like you already have some downstream work which would verify the effectiveness).

[dwnusbaum]

Ok, I pushed the revert to this PR.

it sounds like you already have some downstream work which would verify the effectiveness

I think the updates to RealJenkinsRuleTest.restart here are enough the validate the fix and prevent future regressions.

[jglick]

the updates to RealJenkinsRuleTest.restart here are enough the validate the fix

You mean, when run on Windows.

[dwnusbaum]

You mean, when run on Windows.

Yes, but the CI build here does test against Windows.