API proposal: New overload for ServiceController.Stop()

[Fs00]

Background and Motivation

Follow-up of #765.
The Stop method of the ServiceController class behaves slightly differently when a service has some dependent services: first of all, it stops the latter ones and waits up to 30 seconds for them to be stopped.
This behavior can not be controlled nor disabled by the user. This poses a problem when the user wants to stop dependent services using their custom logic, which could simply consist in a different waiting timeout like in the example below:

// user manually stops dependent services
foreach (var dependent in serviceController.DependentServices)
{
    dependent.Stop();
    dependent.WaitForStatus(ServiceControllerStatus.Stopped, TimeSpan.FromSeconds(5));
}

serviceController.Stop();  // this still tries to stop dependent services

The call to serviceController.Stop() in the last line will still iterate through the dependent services, checking if they are stopped and stopping them if they're not. This is undesirable for two reasons:

• performance: re-iterating through dependent services is redundant and might be costly if dependents are more than a few ones
• robustness and predictability: what if a dependent service is restarted by a third party after being stopped manually by the user but before the call to serviceController.Stop? Then serviceController.Stop would try to stop that service again with the default timeout of 30 seconds. This can be confusing, but more importantly, not wanted by users who prefer to do that manually.

Proposed API

namespace System.ServiceProcess
{
    public class ServiceController : Component
    {
+        public void Stop(bool stopDependentServices)
    }
}

When the stopDependentServices parameter is set to false, there will be no attempt to stop or check the status of dependent services. In case the service has one or more dependents that are running, Stop(false) will throw an InvalidOperationException with an inner Win32Exception with error code ERROR_DEPENDENT_SERVICES_RUNNING (the current implementation may already do this).

The pre-existing Stop() parameterless overload will delegate to the new one, passing in the value true.

Usage Examples

The example shown above can take advantage of this new overload in the following way:

foreach (var dependent in serviceController.DependentServices)
{
    dependent.Stop();
    dependent.WaitForStatus(ServiceControllerStatus.Stopped, TimeSpan.FromSeconds(5));
}

serviceController.Stop(stopDependentServices: false);  // this won't try to stop dependent services again

Or better, a user can now create a recursive method that stops a service and its whole dependents tree with a custom timeout:

public void StopService(ServiceController service)
{
    foreach (var dependent in service.DependentServices)
        StopService(dependent);

    service.Stop(stopDependentServices: false);
    service.WaitForStatus(ServiceControllerStatus.Stopped, TimeSpan.FromSeconds(5));
}

Alternative Design

We could avoid the boolean parameter flag in the public API (which is not best for design and readability) by making the Stop(bool) overload private and instead exposing a parameterless method that delegates to Stop(false).
The thing is, I'm not sure on how would I call such method. StopServiceOnly is the only name that came to my mind, but it still wouldn't be as descriptive as invoking the Stop(bool) overload specifying the name of the parameter, such as Stop(stopDependentServices: false).
I'm open to any suggestion on this.

Risks

None I think, since the original behavior is preserved and the user must opt-in to customize it.

[Anipik]

@Fs00 can you modify the description to fill this template described here https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md

You can mention both the implementation details in the descripition.
After that i can take this up with api-reviewers and they will make the final decision.

[danmoseley]

@Fs00 we actually have an issue template for API proposals now, if you like you can use that, but instead of opening a new issue, paste into the top post of this one.

https://github.com/dotnet/runtime/issues/new?assignees=&labels=api-suggestion&template=02_api_proposal.md&title=

[Fs00]

@Anipik @danmosemsft Just updated the first comment with the template you linked. Let me know if you would like to see some other changes.

[danmoseley]

@Anipik is this a must have to ship 5.0? My reading is that this should be in the Future milestone. If it get into 5.0, fine.

[Anipik]

If it get into 5.0, fine.

that sounds good to me.

[Fs00]

@Anipik Can this be marked as ready for API review?

[KalleOlaviNiemitalo]

For UI scenarios, it might be useful to have public void Stop(CancellationToken cancellationToken) as well. However, that would then have to throw OperationCanceledException rather than TimeoutException. I suppose such an overload can be added in a separate issue if there is demand.

public void Stop(TimeSpan dependentServicesStopTimeout)

The current 30-second timeout applies to each dependent service individually, so the total time spent stopping all dependent services can be much longer. Would dependentServicesStopTimeout be treated in the same way? The plural in the name may give the impression that it is the total timeout.

[KalleOlaviNiemitalo]

If overloads with CancellationToken are ever added, which doesn't have to happen as part of this issue, then I think the least surprising API would have four Stop methods:

 namespace System.ServiceProcess
 {
     public class ServiceController : Component
     {
         public void Stop();
+        public void Stop(TimeSpan dependentServicesStopTimeout);
+        public void Stop(CancellationToken cancellationToken);
+        public void Stop(TimeSpan dependentServicesStopTimeout, CancellationToken cancellationToken);
     }
 }

TimeSpan dependentServicesStopTimeout would apply to each dependent service and default to 30 seconds. Exceeding it would cause TimeoutException. Timeout.InfiniteTimeSpan would remove the limit.

CancellationToken cancellationToken would apply to the whole tree of services and default to CancellationToken.None. Exceeding it would cause OperationCanceledException.

[Fs00]

@KalleOlaviNiemitalo You brought up several interesting points, I'll try to answer them one by one.

The current 30-second timeout applies to each dependent service individually, [...]. Would dependentServicesStopTimeout be treated in the same way? The plural in the name may give the impression that it is the total timeout.

Yes, it would be treated in the same way. And yes, its name is not perfect but I find it hard to find a good alternative (maybe something like stopTimeoutPerDependentService could be better?).

Also, I really like the idea of the CancellationToken overload because it gives more flexibility, even though it might seem a bit overkill for this API imho.

If overloads with CancellationToken are ever added, which doesn't have to happen as part of this issue, then I think the least surprising API would have four Stop methods

I think that .NET team should choose only one of the two approaches, otherwise the API would quickly become confusing.

What if we replace my alternative design in the proposal (which is not very interesting anyway) with your CancellationToken overload? It seems to me that both ideas aim to solve very similar problems and that it makes sense to have only one of the two.

[GSPP]

An alternative design could be:

bool StopDependentServices(TimeSpan dependentServicesStopTimeout, CancellationToken cancellationToken);

And the bool return value would indicate if a timeout occurred.

A caller interested in a timeout could first call StopDependentServices before calling Stop.

[Fs00]

@Anipik Sorry for bothering you again, but I find the CancellationToken overload proposed by @KalleOlaviNiemitalo a very interesting alternative design for this proposal.
I would ask you to remove the label api-ready-for-review until I ping you again. 😅

[KalleOlaviNiemitalo]

I think it would be confusing if both serviceController.Stop() and serviceController.Stop(CancellationToken.None) were valid but not equivalent.

[Fs00]

I think it would be confusing if both serviceController.Stop() and serviceController.Stop(CancellationToken.None) were valid but not equivalent.

I understood that they would be equivalent in your proposal, or did I miss something?

[KalleOlaviNiemitalo]

Oh, I thought you were only going to propose adding public void Stop(CancellationToken cancellationToken), not public void Stop(TimeSpan dependentServicesStopTimeout, CancellationToken cancellationToken).

[Fs00]

My idea is to propose public void Stop(CancellationToken cancellationToken) as an alternative of public void Stop(TimeSpan dependentServicesStopTimeout), tell me if it makes sense to you and if you are OK with that @KalleOlaviNiemitalo

[Anipik]

no worries, let me know when u r ready

[KalleOlaviNiemitalo]

@Fs00, do you mean public void Stop(CancellationToken cancellationToken) would still have the 30-second per-service timeout? That is:

• If a dependent service does not stop in 30 seconds, then throw TimeoutException with the name of that service. This detects buggy services and prevents infinite waits.
• If cancellation is requested via cancellationToken, then throw OperationCanceledException without the name of any service. This lets an interactive user cancel the Stop operation.
• The existing Stop() overload is equivalent to Stop(CancellationToken.None), so it never throws OperationCanceledException, and the API remains compatible with older apps.

I am a bit worried about what would happen after the OperationCanceledException. Stop(CancellationToken) may already have stopped some services but the app does not know which ones those were. Although this concern applies to Stop() with TimeoutException as well, it is less serious there because it only happens if a service is misbehaving. I suppose that, if the user is trying to cancel the operation and close the app, then neither Stop(CancellationToken) nor the app should try to restart those services automatically. If the names of the stopped services (and the service that ServiceController last asked to stop) were reported back to the application (e.g. in Exception.Message, or via a new parameter of Stop), then the app could at least log them. However, if it is an interactive app, then perhaps nobody would read the log anyway; and if the app is not interactive, then I guess it won't provide a CancellationToken in the first place. Perhaps then, it is best not to attempt to solve that; an app that really cares can stop the dependent services itself and have full control.

If you're adding public void Stop(CancellationToken cancellationToken), and it needs void WaitForStatus(ServiceControllerStatus desiredStatus, TimeSpan timeout, CancellationToken cancellationToken) internally, then you could add that to the public API as well.

[KalleOlaviNiemitalo]

Oh, it looks like the current implementation of ServiceController.WaitForStatus treats Timeout.InfiniteTimeSpan as negative rather than infinite, and throws TimeoutException after checking the status just once.

runtime/src/libraries/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

Lines 893 to 910 in cf258a1

	/// Waits until the service has reached the given status or until the specified time
	/// has expired
	public void WaitForStatus(ServiceControllerStatus desiredStatus, TimeSpan timeout)
	{
	if (!Enum.IsDefined(typeof(ServiceControllerStatus), desiredStatus))
	throw new ArgumentException(SR.Format(SR.InvalidEnumArgument, nameof(desiredStatus), (int)desiredStatus, typeof(ServiceControllerStatus)));
	DateTime start = DateTime.UtcNow;
	Refresh();
	while (Status != desiredStatus)
	{
	if (DateTime.UtcNow - start > timeout)
	throw new System.ServiceProcess.TimeoutException(SR.Format(SR.Timeout, ServiceName));
	_waitForStatusSignal.WaitOne(250);
	Refresh();
	}
	}

Making it treat Timeout.InfiniteTimeSpan as infinite might now be a breaking change, and TimeSpan.MaxValue can already be used as a workaround.

[Fs00]

I am a bit worried about what would happen after the OperationCanceledException. Stop(CancellationToken) may already have stopped some services but the app does not know which ones those were.

This is a very reasonable concern, I agree with you on this.

Perhaps then, it is best not to attempt to solve that; an app that really cares can stop the dependent services itself and have full control.

Full control. I think that's the main point of all this. The problem is that the Stop() method as it is now tries to stop all dependent services and there is no way to prevent or control this behavior. You surely can stop all dependent services by yourself (with the ability to choose your custom timeout) before calling Stop(), but the latter will always try to stop dependents even you have already stopped them.

Regarding my originally proposed overload, public void Stop(TimeSpan dependentServicesStopTimeout), I don't like the fact that it leaks an implementation detail of the method. But we don't have to actually do that. Instead, what if we add the following overload:

public void Stop(bool stopDependentServices)

This overload would be called by those users that want to stop dependent services by themselves (passing in false).
I know, boolean parameters are ugly and unclear when their name is not specified inline, but I think we don't have many alternatives. We could hide this overload by wrapping it in a public parameterless method, but I need to find a decent name for it.

What do you think? @KalleOlaviNiemitalo

PS: WaitForStatus(ServiceControllerStatus desiredStatus, TimeSpan timeout, CancellationToken cancellationToken) might be worth a dedicated proposal for it, it's a very nice solution for UI scenarios.

[KalleOlaviNiemitalo]

Yes, public void Stop(bool stopDependentServices) and public WaitForStatus(ServiceControllerStatus desiredStatus, TimeSpan timeout, CancellationToken cancellationToken) together would let an application provide a nice UI with progress and cancellation. This combination looks good to me, better than requiring the application to poll ServiceController.Status on its own.

If ControlService SERVICE_CONTROL_STOP fails, then the current ServiceController.Stop() implementation throws InvalidOperationException with an inner Win32Exception. ServiceController.Stop(bool) would presumably do the same. So, if an application first stopped dependent services and then called ServiceController.Stop(false), but one of the dependent services had been restarted (e.g. by a service trigger event) in the meantime, then the application would be able to detect this specific case by checking for ERROR_DEPENDENT_SERVICES_RUNNING in the Win32Exception.

[KalleOlaviNiemitalo]

better than requiring the application to poll ServiceController.Status on its own

Mainly because ServiceController.WaitForStatus might eventually be changed to use NotifyServiceStatusChangeW instead of polling every 250 milliseconds, as planned in https://github.com/dotnet/corefx/pull/627/files#r24206059. I don't think there currently is a GitHub issue for that feature request, though.

[Fs00]

Ok, I will replace my proposal with public void Stop(bool stopDependentServices). I'll let you make one for public WaitForStatus(ServiceControllerStatus desiredStatus, TimeSpan timeout, CancellationToken cancellationToken) if you want 😉

Mainly because ServiceController.WaitForStatus might eventually be changed to use NotifyServiceStatusChangeW instead of polling every 250 milliseconds, as planned in https://github.com/dotnet/corefx/pull/627/files#r24206059. I don't think there currently is a GitHub issue for that feature request, though.

I don't want to bother you, but would you like to create an issue for that since you pointed it out? 🙂
Created #45049.

[Fs00]

@Anipik Proposal updated. If you're ok with it, this can be marked as ready for review.

[terrajobst]

Video

• Looks good

namespace System.ServiceProcess
{
    public partial class ServiceController
    {
        public void Stop(bool stopDependentServices)
    }
}

[danmoseley]

Is anyone interested in offering an implementation+tests?

[Fs00]

@danmosemsft Count me in! 😃

[danmoseley]

It's assigned to you @Fs00 .. reach out here if you have questions. thanks!