Design Issues in Unix versions of Stop/Restart-Computer cmdlets

# Code

https://github.com/PowerShell/PowerShell/blob/b7cb335f03fe2992d0cbd61699de9d9aafa1d7c1/src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs#L1-L159

I have several questions on these cmdlets... Below is a summarised list of issues with the current code.

## 1. There is no null check for `_process.Dispose()` in the cmdlets' `Dispose()` methods

... despite the fact that the test hook quite clearly prevents `_process` from ever being set if it's enabled. I'm almost as confused by this as I am by the fact that this doesn't make tests fail -- I suspect it's simply taking long enough to call its Dispose() method that the test it was executing inside of has already completed and the error goes unnoticed.

## 2. The internal `RunCommand()` method has multiple design issues

The parameters it exposes are _never used_, and it uses hardcoded values instead.As a result, the different parameters provided by `Stop-Computer` and `Restart-Computer` are completely ignored, and thus the two commands do the exact same thing.

## 3. The testing method.

I acknowledge that it is rather difficult to test stopping/restarting a computer without causing interruptions in the test pipeline, but the current state of tests simply don't test _anything_ apart from basic parameter binding, since all other functionality in the cmdlet is completely skipped when the test hook is enabled.

We need a more robust way of testing these cmdlets; I think the lack of adequate tests is part of the reason the issues in these cmdlets went unnoticed.

## 4. `IDisposable` is not implemented properly

According to the documented [recommended pattern](https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose) there are two additional members that should be implemented as part of the IDisposable pattern, so that multiple calls to Dispose() effectively no-op after the first call.

# Related Issues/PRs

#5448 and #11151

---

This code is still present in the current master branch of the repo and was included in 7.1-preview1, but it's still very much unfinished. Are there plans to finish it, at least to a minimum viable product?

	// Copyright (c) Microsoft Corporation.
	// Licensed under the MIT License.

	#if UNIX

	using System;
	using System.Diagnostics;
	using System.Management.Automation;
	using System.Management.Automation.Internal;
	using System.Runtime.InteropServices;

	namespace Microsoft.PowerShell.Commands
	{
	#region Restart-Computer

	/// <summary>
	/// Cmdlet to restart computer.
	/// </summary>
	[Cmdlet(VerbsLifecycle.Restart, "Computer", SupportsShouldProcess = true,
	HelpUri = "https://go.microsoft.com/fwlink/?LinkID=2097060", RemotingCapability = RemotingCapability.SupportedByCommand)]
	public sealed class RestartComputerCommand : CommandLineCmdletBase
	{
	// TODO: Support remote computers?

	#region "Overrides"

	/// <summary>
	/// BeginProcessing.
	/// </summary>
	protected override void BeginProcessing()
	{
	if (InternalTestHooks.TestStopComputer)
	{
	var retVal = InternalTestHooks.TestStopComputerResults;
	if (retVal != 0)
	{
	string errMsg = StringUtil.Format("Command returned 0x{0:X}", retVal);
	ErrorRecord error = new ErrorRecord(
	new InvalidOperationException(errMsg), "Command Failed", ErrorCategory.OperationStopped, "localhost");
	WriteError(error);
	}
	return;
	}

	RunCommand("/sbin/shutdown", "-r now");
	}
	#endregion "Overrides"
	}
	#endregion Restart-Computer

	#region Stop-Computer

	/// <summary>
	/// Cmdlet to stop computer.
	/// </summary>
	[Cmdlet(VerbsLifecycle.Stop, "Computer", SupportsShouldProcess = true,
	HelpUri = "https://go.microsoft.com/fwlink/?LinkID=2097151", RemotingCapability = RemotingCapability.SupportedByCommand)]
	public sealed class StopComputerCommand : CommandLineCmdletBase
	{
	// TODO: Support remote computers?

	#region "Overrides"

	/// <summary>
	/// BeginProcessing.
	/// </summary>
	protected override void BeginProcessing()
	{
	var args = "-P now";
	if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
	{
	args = "now";
	}
	if (InternalTestHooks.TestStopComputer)
	{
	var retVal = InternalTestHooks.TestStopComputerResults;
	if (retVal != 0)
	{
	string errMsg = StringUtil.Format("Command returned 0x{0:X}", retVal);
	ErrorRecord error = new ErrorRecord(
	new InvalidOperationException(errMsg), "Command Failed", ErrorCategory.OperationStopped, "localhost");
	WriteError(error);
	}
	return;
	}

	RunCommand("/sbin/shutdown", args);
	}
	#endregion "Overrides"
	}

	/// <summary>
	/// A base class for cmdlets that can run shell commands.
	/// </summary>
	public class CommandLineCmdletBase : PSCmdlet, IDisposable
	{
	#region Private Members
	private Process _process = null;
	#endregion

	#region "IDisposable Members"

	/// <summary>
	/// Dispose Method.
	/// </summary>
	public void Dispose()
	{
	_process.Dispose();
	}

	#endregion "IDisposable Members"

	#region "Overrides"
	/// <summary>
	/// To implement ^C.
	/// </summary>
	protected override void StopProcessing()
	{
	if (_process == null) {
	return;
	}

	try {
	if (!_process.HasExited) {
	_process.Kill();
	}
	WriteObject(_process.ExitCode);
	}
	catch (InvalidOperationException) {}
	catch (NotSupportedException) {}
	}
	#endregion "Overrides"

	#region "Internals"

	/// <summary>
	/// Run a command.
	/// </summary>
	protected void RunCommand(String command, String args) {
	String cmd = "";

	_process = new Process()
	{
	StartInfo = new ProcessStartInfo
	{
	FileName = "/sbin/shutdown",
	Arguments = cmd,
	RedirectStandardOutput = false,
	UseShellExecute = false,
	CreateNoWindow = true,
	}
	};
	_process.Start();
	}
	#endregion
	}
	#endregion
	}
	#endif

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Design Issues in Unix versions of Stop/Restart-Computer cmdlets #12482

Code

1. There is no null check for `_process.Dispose()` in the cmdlets' `Dispose()` methods

2. The internal `RunCommand()` method has multiple design issues

3. The testing method.

4. `IDisposable` is not implemented properly

Related Issues/PRs

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Design Issues in Unix versions of Stop/Restart-Computer cmdlets #12482

Description

Code

1. There is no null check for _process.Dispose() in the cmdlets' Dispose() methods

2. The internal RunCommand() method has multiple design issues

3. The testing method.

4. IDisposable is not implemented properly

Related Issues/PRs

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

1. There is no null check for `_process.Dispose()` in the cmdlets' `Dispose()` methods

2. The internal `RunCommand()` method has multiple design issues

4. `IDisposable` is not implemented properly