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

[Discussion] ServiceController API needs some improvements #765

Closed
Fs00 opened this issue Dec 11, 2019 · 10 comments
Closed

[Discussion] ServiceController API needs some improvements #765

Fs00 opened this issue Dec 11, 2019 · 10 comments

Comments

@Fs00
Copy link
Contributor

Fs00 commented Dec 11, 2019

I've been using the ServiceController class for some time now in an application of mine and I personally find that some features of the API could be improved. Here they are:

  • Timeout for stopping dependent services in Stop() is extremely long (30 secs).
    Which service takes even half of that time to stop itself? In my experience, when a stop request timed out instead of taking a couple of seconds to complete, it was because the dependent service somehow ignored it and refused to stop, causing my application to wait 30 seconds uselessly without having the ability to prevent it.
    I would suggest to reduce the timeout to something around 10 secs and/or add an overload to Stop that accepts a TimeSpan that specifies the timeout.
  • Stop method could be more convenient to use.
    The function DoStopSvc in this official WinAPI example checks if a stop is already pending before sending the stop control to the service, and in any case it waits for the service to stop before returning. Could ServiceController.Stop do the same? Waiting for a service to become stopped after a stop request is such a common use case that would be very convenient if Stop did it (it already does it when stopping dependent services).
  • System.ServiceProcess.TimeoutException message lacks the name of the service.
    The Stop method can raise a TimeoutException when a dependent service can't be stopped in time. The problem is that the exception doesn't tell which is the service that caused the timeout. Could this information be added in the message?

If you decide it's ok to proceed with these changes, I can take care of implementing them (for the last one I'd need some guidance on how to change exception messages resources).

PS: I wasn't sure whether to split this issue in multiple ones at this time since it is meant as a discussion on the topic.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.ServiceProcess untriaged New issue has not been triaged by the area owner labels Dec 11, 2019
@danmoseley
Copy link
Member

We would certainly be interested in a PR that makes the exception(s) more useful.

@Fs00
Copy link
Contributor Author

Fs00 commented Mar 1, 2020

Ok, I'll look into it when I have some time. Can you point me in the right direction on how to change/add new exception message resources? (is some special operation needed?)
And what about the other two proposals?

@danmoseley
Copy link
Member

No special process for the exceptions..just a PR.

@Anipik is owner of this area and could comment on the other two suggestions

@Anipik
Copy link
Contributor

Anipik commented Mar 2, 2020

@Fs00 I will take a look at the two proposals and update the issue.

@Fs00
Copy link
Contributor Author

Fs00 commented Apr 21, 2020

@Anipik Any news on those two proposals?

@Anipik
Copy link
Contributor

Anipik commented Apr 21, 2020

Any news on those two proposals?

i will get back to u by tonight

@Anipik
Copy link
Contributor

Anipik commented Apr 22, 2020

and/or add an overload to Stop that accepts a TimeSpan that specifies the timeout.

You can write api proposal for it and we will happy to take it. Here is goo example of an proposal #15725
Changing the default value of 30 secs would be an issue as it might break some body else.

Stop method could be more convenient to use.

Feel free to throw up a pr to improve this.

@Fs00
Copy link
Contributor Author

Fs00 commented Apr 22, 2020

@Anipik After some thoughts, I've decided that it's better for consistency and compatibility to leave the Stop() method as-is. Instead, I've made a proposal for a new Stop overload with the improvements I'd like to see.

@Anipik Anipik added this to the Future milestone Jun 24, 2020
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jul 9, 2020
MichalStrehovsky pushed a commit to MichalStrehovsky/runtime that referenced this issue Mar 25, 2021
Merge from dotnet/runtime:main
@Fs00
Copy link
Contributor Author

Fs00 commented Jun 15, 2021

#33927 and #52519 solved the biggest pain points of this API, therefore I think that this issue can be closed now.
Thank you all for your guidance! It's been great to be able to contribute 😃

@Fs00 Fs00 closed this as completed Jun 15, 2021
@danmoseley
Copy link
Member

@Fs00 thank you for your contributions! We have many other issues (mostly marked 'up-for-grabs') in case you are interested in continuing to contribute. Perhaps there is another bug you feel passionately about.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants