Fix exception and enable event logging when service has no name

[danmoseley]

Fix #62923

While all Windows services must have a non empty name, if you're starting a service in its own process, such that it's unambiguous which service you want to start, you do not need to pass that name. Therefore ServiceBase does not require you set ServiceName. However if ServiceBase is configured to write to the event log, which it is by default, it will fail because it uses the ServiceName as the source name. The result is an exception, and no expected messages, in the event log.

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

Line 260 in 62b34d9

Source = ServiceName

The Microsoft.Extensions.Hosting library makes it easy to pass no service name to ServiceBase:

runtime/src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/WindowsServiceLifetimeHostBuilderExtensions.cs

Line 31 in 62b34d9

return UseWindowsService(hostBuilder, _ => { });

runtime/src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/WindowsServiceLifetime.cs

Lines 35 to 40 in 62b34d9

	if (windowsServiceOptionsAccessor == null)
	{
	throw new ArgumentNullException(nameof(windowsServiceOptionsAccessor));
	}
	_hostOptions = optionsAccessor.Value;
	ServiceName = windowsServiceOptionsAccessor.Value.ServiceName;

runtime/src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/WindowsServiceLifetimeOptions.cs

Line 13 in 62b34d9

public string ServiceName { get; set; } = string.Empty;

The fix as indicated by @ericstj is for ServiceBase to populate its empty ServiceName using the value passed from the SCM in its callback.

• Increase timeouts that are sporadically being hit per test history data
• Add logging to TestServiceInstaller
• Update ServiceTypeOptions to include others present in winnt.h, but commented out
• Rename SERVICE_ constants to match winnt.h (it's less confusing)
• Remove defunct comment
• Add failing test
• Remove event log noise
• Make test pass

[ghost]

Tagging subscribers to this area: @dotnet/area-system-serviceprocess
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fix #62923

While all Windows services must have a non empty name, if you're starting a service in its own process, such that it's unambiguous which service you want to start, you do not need to pass that name. Therefore ServiceBase does not require you set ServiceName. However if ServiceBase is configured to write to the event log, which it is by default, it will fail because it uses the ServiceName as the source name. The result is an exception, and no expected messages, in the event log.

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

Line 260 in 62b34d9

Source = ServiceName

The Microsoft.Extensions.Hosting library makes it easy to pass no service name to ServiceBase:

runtime/src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/WindowsServiceLifetimeHostBuilderExtensions.cs

Line 31 in 62b34d9

return UseWindowsService(hostBuilder, _ => { });

runtime/src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/WindowsServiceLifetime.cs

Lines 35 to 40 in 62b34d9

	if (windowsServiceOptionsAccessor == null)
	{
	throw new ArgumentNullException(nameof(windowsServiceOptionsAccessor));
	}
	_hostOptions = optionsAccessor.Value;
	ServiceName = windowsServiceOptionsAccessor.Value.ServiceName;

runtime/src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/WindowsServiceLifetimeOptions.cs

Line 13 in 62b34d9

public string ServiceName { get; set; } = string.Empty;

The fix as indicated by @ericstj is for ServiceBase to populate its empty ServiceName using the value passed from the SCM in its callback.

• Increase timeouts that are sporadically being hit per test history data
• Add logging to TestServiceInstaller
• Update ServiceTypeOptions to include others present in winnt.h, but commented out
• Rename SERVICE_ constants to match winnt.h (it's less confusing)
• Remove defunct comment
• Add failing test
• Remove event log noise
• Make test pass

Author:	danmoseley
Assignees:	-
Labels:	area-System.ServiceProcess
Milestone:	-

[danmoseley]

@dotnet/dnceng Helix error:

.packages/microsoft.dotnet.helix.sdk/7.0.0-beta.22179.1/tools/xharness-runner/XHarnessRunner.targets(122,5): error MSB4018: The "CreateXHarnessAppleWorkItems" task failed unexpectedly.
System.Net.Http.HttpRequestException: The SSL connection could not be established, see inner exception.
 ---> System.Security.Authentication.AuthenticationException: The remote certificate is invalid because of errors in the certificate chain: RevocationStatusUnknown

[premun]

@danmoseley the SSL problem is already fixed in Arcade but runtime hasn't consumed the .NET 7.0 Arcade yet:
#67349

[jkotas]

This ends up calling

runtime/src/libraries/System.Private.CoreLib/src/System/ComponentModel/Win32Exception.cs

Line 46 in 441271c

public Win32Exception(string? message) : this(Marshal.GetLastPInvokeError(), message)

that will read the last PInvoke error that can be corrupted by now.

[danmoseley]

Ugh - Win32Exception is a bit of a trap. So is this the correct pattern to achieve this?

Win32Exception ex = new Win32Exception(); 
throw new Win32Exception(ex.NativeErrorCode, $"Could not delete service '{ServiceName}'. {ex.Message}");

[jkotas]

Yes, that is correct. I think writing it like this looks better:

int errorCode = Marshal.GetLastWin32Error();
string errorMessage = new Win32Exception(errorCode).Message;
throw new Win32Exception(errorCode, $"Could not delete service '{ServiceName}'. {errorMessage}");

[danmoseley]

similar
https://github.com/danmoseley/runtime/blob/a431bbc76dd80cb6f960725547ad3ccb5c9cf679/src/libraries/System.Drawing.Common/src/misc/DbgUtil.cs#L57-L59

I wonder what would be an appropriate new API to make this (seemingly reasonable) pattern easier.

throw Win32Exception.WithMessagePrefix($"Could not delete service '{ServiceName}.');

?

[danmoseley]

Actually, the above suggestion would still run string formatting machinery before reading the last error.

[jkotas]

I think an API that forces you to call Marshal.GetLastWin32Error yourself and gives you the error message for the OS error code would be more flexible.

I think the current behavior of Win32Exception constructors that call Marshal.GetLastWin32Error for you is very error prone.

[danmoseley]

Hmm, so eg this? (assuming I want the code in the exception object too)

int errorCode = Marshal.GetLastPInvokeError();
string errorMessage = Marshal.GetMessageForPInvokeError(errorCode);
throw new Win32Exception(errorCode, $"Could not delete service '{ServiceName}'. {errorMessage}");

[jkotas]

Yes. (The ServiceController project targets netstandard, so you won't be able to use this here actually.)

[danmoseley]

created #67872

[danmoseley]

Any other feedback?

[danmoseley]

Interpreter failure is #67880

[ericstj]

LGTM, thank you for fixing this!