Add ServiceBase #22920

weshaggard · 2017-08-03T17:11:55Z

This change add ServiceBase and some of the missing ServiceController APIs that are in .NET Framework.

resolves https://github.com/dotnet/corefx/issues/6024

The ported ServiceBase matches desktop except the dependency on EventLog is removed as we don't have EventLog support in .NET Core (at least not yet).

The port doesn't include ServiceInstaller or ServiceProcessInstaller as we don't want to pull forward the entire Installer abstraction into .NET Core. So while we have ServiceBase a .NET Core hosted service will need to be installed/removed using other methods (sc.exe, installers, or via code like our test service).

As part of the testing work I removed our dependency on the old native service and started using our new managed service which uses ServiceBase.

PTAL @stephentoub @ericstj

@TravisTheTechie @dasMulli it would be great if you guys could take a look as well.

TODO: - Need to add tests Porting Notes: - Excludes EventLog dependency - Removes latebound dependency on messagebox Doesn't include ServiceInstaller but for testing purposes ported parts of it into TestServiceInstaller class that will be used for testing ServiceBase.

…Base

The managed TestService now has a simple ServiceBase TestService that logs all the events it recieves to a file. The project also as a simple TestServiceInstaller that will allow for programatic install and removal of a service. The main entry point provides a simple way to create/remove/run the TestService.

Update test project to use the new managed TestService instead of the old native test service and adjust the existing tests to account for those changes. Add basic ServiceBase tests. It isn't possible to test all the different events so only test the ones that we can reliably tests. The communication between the service and the tests is via a text file which isn't ideal but it is the simpliest option of the various options to work. All the tests need to call Stop to ensure the log file is flushed to disk and we have all the content we need in it.

…eption

weshaggard · 2017-08-03T17:18:06Z

test Outerloop NETFX x86 Release Build
test Outerloop Windows x64 Debug Build

justinvp · 2017-08-03T17:55:58Z

src/Common/src/Interop/Windows/advapi32/Interop.SERVICE_DELAYED_AUTOSTART_INFO.cs

+        public struct SERVICE_DELAYED_AUTOSTART_INFO
+        {
+            public bool fDelayedAutostart;
+        };


Nit: Unnecessary semicolon

justinvp · 2017-08-03T17:56:04Z

src/Common/src/Interop/Windows/advapi32/Interop.SERVICE_DESCRIPTION.cs

+        public struct SERVICE_DESCRIPTION
+        {
+            public IntPtr description;
+        };


Nit: Unnecessary semicolon

justinvp · 2017-08-03T17:58:43Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceBase.cs

+                return false;
+
+            // no slashes or backslash allowed
+            foreach (char c in serviceName.ToCharArray())


ToCharArray() unnecessarily allocates a char array

justinvp · 2017-08-03T18:00:07Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceBase.cs

+                    Initialize(true);
+                }
+
+                if (Environment.OSVersion.Version.Major >= 5)


Is this check still needed?

justinvp · 2017-08-03T18:00:22Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceBase.cs

+                    _status.controlsAccepted = _status.controlsAccepted | AcceptOptions.ACCEPT_SHUTDOWN;
+                }
+
+                if (Environment.OSVersion.Version.Major < 5)


Is this still needed?

justinvp · 2017-08-03T18:01:58Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

+            set
+            {
+                if (value == null)
+                    throw new ArgumentNullException("value");


nameof(value)

justinvp · 2017-08-03T18:02:04Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

+                if (value == null)
+                    throw new ArgumentNullException("value");
+
+                if (string.Compare(value, _name, StringComparison.OrdinalIgnoreCase) == 0)


string.Equals

justinvp · 2017-08-03T18:02:32Z

...iceProcess.ServiceController/src/System/ServiceProcess/ServiceProcessDescriptionAttribute.cs

+//     Copyright (c) Microsoft Corporation.  All rights reserved.
+// </copyright>                                                                
+//------------------------------------------------------------------------------
+


Incorrect license header

Thanks for pointing out. I decided to not include this file at all but I forget to remove it from my original commit.

justinvp · 2017-08-03T18:03:03Z

...iceProcess.ServiceController/src/System/ServiceProcess/ServiceProcessDescriptionAttribute.cs

+
+
+    using System;
+    using System.ComponentModel;   


Run the codeformatter? Usings shouldn't be nested.

justinvp · 2017-08-03T18:03:39Z

...ystem.ServiceProcess.ServiceController/src/System/ServiceProcess/SessionChangeDescription.cs

+    public struct SessionChangeDescription
+    {
+        private SessionChangeReason _reason;
+        private int _id;


Nit: readonly

dasMulli · 2017-08-03T18:27:28Z

...Controller/tests/System.ServiceProcess.ServiceController.TestService/TestServiceInstaller.cs

+                    }
+                }
+            }
+            //catch { }


I guess this should be removed or commented back in (along with //try)

weshaggard · 2017-08-03T20:21:54Z

@dotnet/dnceng what is the best way to determine which tests failed? I see https://mc.dot.net/#/user/weshaggard/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/57c16a39b435caccc19aa518ab8b527d98da83e9 but don't see any failures just some "Unknown" blocks. This is from https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/24/.

MattGal · 2017-08-03T20:24:47Z

@weshaggard the ?s are actually tests that didn't run at all for that configuration (no matching work item) but til recently we had a bug that showed them as ?. (@ChadNedzlek I believe has already fixed this but it hasn't been rolled out)

I'll take a look since it's definitely showing runs that I'd label as complete and passed as failures, then hand off to the FR team as needed.

MattGal · 2017-08-03T20:47:46Z

@weshaggard this is freaky, there's some kind of MC bug. The failing test in all cases of those 4 runs is System.Runtime.Performance.Tests, which is timing out. Example log here.

I will raid a FR issue and tag you / Chad. There's something definitely wrong about how this is getting rendered (I'm guessing there have been several builds of this, one of which succeeded, and stomped finding that)

edit - these comments were before I realized job restarting had happened

weshaggard · 2017-08-03T20:53:26Z

(I'm guessing there have been several builds of this, one of which succeeded, and stomped finding that)

Yes that is why I have an open request to split the results across the different legs.

weshaggard · 2017-08-04T15:56:57Z

test Linux x64 Release Build

We have always had a "not supported" netstandard fallback but we put the real implementation in a netstandard windows build but that is also a little bit of a lie because we don't support it on things like UWP. So instead the real implementation should be under netcoreapp windows which is the only one we really support.

We no longer need to skip because the tests are now netcoreapp and netfx specific so we won't try to run them on uap now.

ericstj · 2017-08-04T20:31:43Z

src/System.ServiceProcess.ServiceController/ref/Configurations.props

@@ -2,8 +2,9 @@
 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <BuildConfigurations>
-      netstandard;
+      netcoreapp;


Why was this needed?

No I will remove it and just leave netstandard and netfx given we have to have a netstandard ref now for compat with the older package.

ericstj · 2017-08-04T20:36:17Z

src/System.ServiceProcess.ServiceController/src/Configurations.props

@@ -2,7 +2,7 @@
 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <BuildConfigurations>
-      netstandard-Windows_NT;
+      netcoreapp-Windows_NT;


This means UWP no longer gets a valid impl. I guess that's OK. I doubt any of the API would work from appcontainer.

correct UWP isn't supporting these. On top of the win32 APIs it requires admin access for most of these actions.

ericstj

LGTM modulo the extraneous netcoreapp ref config

We only need a netsteandard ref that can be shared between netcoreapp and netstandard.

Add ServiceBase Commit migrated from dotnet/corefx@bdb9164

weshaggard added 10 commits August 3, 2017 10:00

Commit ServiceBase.cs unmodified for diffing purposes

7f8b78e

Update ServiceController APIs to match .NET Framework

c4e2ca4

Switch to using ThreadPool instead of delegate.BeginInvoke in Service…

dce71b5

…Base

Fix netfx ref build for System.ServiceProcess.ServiceController

089a449

Remove native test service project as we no longer need it

88783b6

Move ServiceController test project into the root tests folder.

ddc1c61

Update ServiceController project/solution configurations

a0243f3

weshaggard added the area-System.ServiceProcess label Aug 3, 2017

weshaggard added this to the 2.1.0 milestone Aug 3, 2017

weshaggard self-assigned this Aug 3, 2017

dnfclas added the cla-already-signed label Aug 3, 2017

Make TimeoutException match .NET Framework and inherit from SystemExc…

57c16a3

…eption

justinvp reviewed Aug 3, 2017

View reviewed changes

dasMulli reviewed Aug 3, 2017

View reviewed changes

Address feedback for ServiceBase PR

4698785

weshaggard added 2 commits August 4, 2017 13:11

Remove SkipOnFramework UAP on the ServiceController tests

f7627de

We no longer need to skip because the tests are now netcoreapp and netfx specific so we won't try to run them on uap now.

ericstj reviewed Aug 4, 2017

View reviewed changes

ericstj approved these changes Aug 4, 2017

View reviewed changes

Only have netstandard ref for System.ServiceProcess.ServiceController

db069e0

We only need a netsteandard ref that can be shared between netcoreapp and netstandard.

weshaggard merged commit bdb9164 into dotnet:master Aug 4, 2017

Tratcher mentioned this pull request Aug 5, 2017

Microsoft.AspNetCore.Hosting.WindowsServices .NET Core Support aspnet/Hosting#904

Closed

scottt732 mentioned this pull request Aug 19, 2017

Get running on dotnet cli / .NET Core Topshelf/Topshelf#277

Closed

henkmollema mentioned this pull request Aug 28, 2017

Cross-compile Windows Services hosting package aspnet/Hosting#1189

Closed

leonard-thieu mentioned this pull request Nov 20, 2017

Port to .NET Core 2.1 leonard-thieu/toofz-services-core#31

Open

leonard-thieu mentioned this pull request Dec 8, 2017

Port projects to .NET Standard/.NET Core leonard-thieu/toofz-necrodancer#39

Open

23 tasks

weshaggard deleted the ServiceBase branch February 9, 2018 17:53

i4004 mentioned this pull request Mar 14, 2018

Simplify.WindowsServices .Net Core support i4004/Simplify#107

Closed

zhenggu mentioned this pull request Jul 20, 2018

Fix Set-Service -Status Stopped runs failed on the service with dependencies PowerShell/PowerShell#5525

Merged

9 tasks

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#22920 from weshaggard/ServiceBase

b65eee6

Add ServiceBase Commit migrated from dotnet/corefx@bdb9164

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ServiceBase #22920

Add ServiceBase #22920

weshaggard commented Aug 3, 2017

weshaggard commented Aug 3, 2017

justinvp Aug 3, 2017

justinvp Aug 3, 2017

justinvp Aug 3, 2017

justinvp Aug 3, 2017

weshaggard Aug 3, 2017

justinvp Aug 3, 2017

weshaggard Aug 3, 2017

justinvp Aug 3, 2017

justinvp Aug 3, 2017

justinvp Aug 3, 2017

weshaggard Aug 3, 2017

justinvp Aug 3, 2017

justinvp Aug 3, 2017

dasMulli Aug 3, 2017

weshaggard commented Aug 3, 2017

MattGal commented Aug 3, 2017 •

edited

Loading

MattGal commented Aug 3, 2017 •

edited

Loading

weshaggard commented Aug 3, 2017

weshaggard commented Aug 4, 2017

ericstj Aug 4, 2017

weshaggard Aug 4, 2017

ericstj Aug 4, 2017

weshaggard Aug 4, 2017

ericstj left a comment

Add ServiceBase #22920

Add ServiceBase #22920

Conversation

weshaggard commented Aug 3, 2017

weshaggard commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weshaggard commented Aug 3, 2017

MattGal commented Aug 3, 2017 • edited Loading

MattGal commented Aug 3, 2017 • edited Loading

weshaggard commented Aug 3, 2017

weshaggard commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj left a comment

Choose a reason for hiding this comment

MattGal commented Aug 3, 2017 •

edited

Loading

MattGal commented Aug 3, 2017 •

edited

Loading