[API Proposal]: Extend ServiceBase to allow handling all service control codes

[levicki]

Background and motivation

Currently, there is OnCustomCommand() method in ServiceBase class which can be overriden and appears to be wired to receive other service control codes sent to underlying native LPHANDLER_FUNCTION_EX callback by the operating system.

However, this method only receives the service control code -- it doesn't receive any other parameters (namely dwEventType and lpEventData) which makes it impossible to actually handle any service control codes that require those such as device notifications, power notifications, etc.

Given that it is possible to call RegisterDeviceNotification() using P/Invoke and pass the native service handle from ServiceBase, it should also be possible to actually receive data that is sent by the OS along with the control code.

I believe this method was misnamed -- it is not receiving commands but service control codes, so I am not sure whether creating an overload with additional parameters is the way to go or if differently named method should be created. I would personally prefer sticking to underlying OS naming, hence the API proposal below.

If it is ever implemented, it would be nice if it was also backported to .Net Framework.

API Proposal

namespace System.ServiceProcess
{
	public class ServiceBase : Component
	{
		protected virtual void HandlerFunctionEx(UInt32 dwControl, UInt32 dwEventType, IntPtr lpEventData, IntPtr lpContext)
		{
		}
	}
}

API Usage

namespace Demo
{
	public static class Win32
	{
		// marshaling omitted for brevity
	}

	partial class DemoService : ServiceBase
	{
		protected override void HandlerFunctionEx(UInt32 dwControl, UInt32 dwEventType, IntPtr lpEventData, IntPtr lpContext)
		{
			Win32.DEV_BROADCAST_HDR dbh = null;

			switch (dwControl) {
			case Win32.SERVICE_CONTROL_DEVICEEVENT: // handle device events
				switch (dwEventType) {
				case Win32.DBT_DEVICEARRIVAL:
					// do something when device is added
					dbh = Marshal.PtrToStructure<Win32.DEV_BROADCAST_HDR>(lpEventData);
					break;
				case Win32.DBT_DEVICEREMOVECOMPLETE:
					// do something when device is removed
					break;
				}
				break;
			}
			// Call base implementation for the rest (i.e. `OnStart()`, `OnStop()`, etc)
			base.HandlerFunctionEx(dwControl, dwEventType, lpEventData, lpContext);
		}
	}
}

Alternative Designs

Alternative would be creating virtual OnDeviceEvent() API and marshaling everything involved in the framework itself, but that would take considerably more effort and I am pretty certain just exposing low level access to all parameters would be enough for users like me who need this kind of functionality in a .Net service.

Risks

No response

[ghost]

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

Issue Details

---

Background and motivation

Currently, there is OnCustomCommand() method in ServiceBase class which can be overriden and appears to be wired to receive other service control codes sent to underlying native LPHANDLER_FUNCTION_EX callback by the operating system.

However, this method only receives the service control code -- it doesn't receive any other parameters (namely dwEventType and lpEventData) which makes it impossible to actually handle any service control codes that require those such as device notifications, power notifications, etc.

Given that it is possible to call RegisterDeviceNotification() using P/Invoke and pass the native service handle from ServiceBase, it should also be possible to actually receive data that is sent by the OS along with the control code.

I believe this method was misnamed -- it is not receiving commands but service control codes, so I am not sure whether creating an overload with additional parameters is the way to go or if differently named method should be created. I would personally prefer sticking to underlying OS naming, hence the API proposal below.

If it is ever implemented, it would be nice if it was also backported to .Net Framework.

API Proposal

namespace System.ServiceProcess
{
	public class ServiceBase : Component
	{
		protected virtual void HandlerFunctionEx(UInt32 dwControl, UInt32 dwEventType, IntPtr lpEventData, IntPtr lpContext)
		{
		}
	}
}

API Usage

namespace Demo
{
	public static class Win32
	{
		// marshaling omitted for brevity
	}

	partial class DemoService : ServiceBase
	{
		protected override void HandlerFunctionEx(UInt32 dwControl, UInt32 dwEventType, IntPtr lpEventData, IntPtr lpContext)
		{
			Win32.DEV_BROADCAST_HDR dbh = null;

			switch (dwControl) {
			case Win32.SERVICE_CONTROL_DEVICEEVENT: // handle device events
				switch (dwEventType) {
				case Win32.DBT_DEVICEARRIVAL:
					// do something when device is added
					dbh = Marshal.PtrToStructure<Win32.DEV_BROADCAST_HDR>(lpEventData);
					break;
				case Win32.DBT_DEVICEREMOVECOMPLETE:
					// do something when device is removed
					break;
				}
				break;
			}
			// Call base implementation for the rest (i.e. `OnStart()`, `OnStop()`, etc)
			base.HandlerFunctionEx(dwControl, dwEventType, lpEventData, lpContext);
		}
	}
}

Alternative Designs

Alternative would be creating virtual OnDeviceEvent() API and marshaling everything involved in the framework itself, but that would take considerably more effort and I am pretty certain just exposing low level access to all parameters would be enough for users like me who need this kind of functionality in a .Net service.

Risks

No response

Author:	levicki
Assignees:	-
Labels:	api-suggestion, area-System.ServiceProcess, untriaged
Milestone:	-

[bartonjs]

Is this something that requires us changing ServiceProcess, or can it easily be done in a derived type?

[levicki]

Is this something that requires us changing ServiceProcess, or can it easily be done in a derived type?

@bartonjs

If I am not mistaken, ServiceProcess is a namespace. Did you perhaps mean ServiceBase instead?

If you meant deriving from that class, that's what you already do when you are writing a windows service in .Net, isn't it?

I don't see a way to override / extend ServiceCommandCallbackEx which would be necessary to wire up the hypothetical OnDeviceEvent method (which I mention in alternative design) like it has been done for OnPowerEvent and OnSessionChange (those are called from DeferredPowerEvent and DeferredSessionChange respectively).

I am not sure if I am explaining what functionality is needed clearly enough -- the form for making an API proposal is asking me to propose an API design where I would rather propose desired functionality so I apologize if I am not making much sense.

[bartonjs]

My bad, I think my eyes were looking at the area label when my fingers started writing ServiceBase, and the eyes apparently rewrote the instructions to the fingers. Bad eyes.

OK, I think I've remembered enough how NT Services work to understand the problem. The only way a derived type can get at the eventType/eventData/eventContext values would be to P/Invoke RegisterServiceCtrlHandlerEx to replace the handler entirely, which almost certainly does bad things to ServiceBase.

One complication in expanding the current functionality is that the class never invokes OnCustomCommand from the service control (calling) thread, so the two pointers are probably invalid by the time that fires. So, implementation-wise, it'd require something like

namespace System.ServiceProcess
{
    public partial class ServiceBase
    {
        // <returns>
        //   <see langword="true"/> if the control was handled, and should not propagate
        //   to deferred processing; otherwise, <see langword="false"/>.
        // </returns>
	protected virtual bool ServiceControlHandler(int command, int eventType, IntPtr eventData, IntPtr eventContext)
	{
            return false;
	}
    }
}

And the current handler would invoke that in the default case before narrowing the funnel. e.g.

                default:
                    {
+                       if (!ServiceControlHandler(command, eventType, eventData, eventContext))
+                       {
-                       ServiceCommandCallback(command);
+                           ServiceCommandCallback(command);
+                       }
+
                        break;
                    }

And the name might warrant a Dangerous or Unsafe or something, since it's (a) part of the a service control pipeline and thus is supposed to be fast and (b) is playing with pointers.

Heck, maybe we should just call a spade a spade and present them as void* instead of IntPtr.

[bartonjs]

If it is ever implemented, it would be nice if it was also backported to .Net Framework.

Unfortunately, that's not possible. It'd be future versions of .NET (nee Core)-only, because .NET Framework is no longer receiving new features (https://docs.microsoft.com/en-us/dotnet/framework/migration-guide/versions-and-dependencies). That was part of the reason behind my question of whether or not it had to be done in ServiceBase... if it was something that could easily be provided by a type deriving from ServiceBase then it could be offered via a NuGet package and apply downlevel.

Since it requires changing (or entirely replacing) ServiceBase that puts .NET Framework out-of-bounds.

[levicki]

@bartonjs

Would perhaps something like this be possible then?

        private int ServiceCommandCallbackEx(int command, int eventType, IntPtr eventData, IntPtr eventContext)
        {
            switch (command)
            {
+                case ControlOptions.CONTROL_DEVICEEVENT:
+                    {
+                        DEV_BROADCAST_HDR dbh = new DEV_BROADCAST_HDR();
+                        Marshal.PtrToStructure(eventData, dbh);
+                        switch (dbh.dbch_devicetype)
+                        {
+                            case DBT_DEVTYP_DEVICEINTERFACE:
+                                {
+                                    DEV_BROADCAST_DEVICEINTERFACE dbi = new DEV_BROADCAST_DEVICEINTERFACE();
+                                    Marshal.PtrToStructure(eventData, dbi);
+                                    ThreadPool.QueueUserWorkItem(_ => DeferredDeviceInterfaceEvent(eventType, dbi));
+                                    break;
+                                }
+                            default:
+                                throw NotImplementedException;
+                        }
+                        break;
+                    }
                case ControlOptions.CONTROL_POWEREVENT:
        ...
+        private void DeferredDeviceInterfaceEvent(int eventType, DEV_BROADCAST_DEVICEINTERFACE dbi)
+        {
+            try {
+                 OnDeviceInterfaceEvent(eventType, dbi);
+             } catch (Exception e) {
+                 // do whatever necessary here
+             }
+        }
        ...
+        protected virtual void OnDeviceInterfaceEvent(int eventType, DEV_BROADCAST_DEVICEINTERFACE dbi)
+        {
+        }

This would explicitly make a copy of the eventData before doing a deferred call thus avoiding IntPtr validity issue.

Derived class could then override OnDeviceInterfaceEvent() and receive device add / remove notifications in a .Net service.

Same thing should be done for DBT_DEVTYP_VOLUME device type and DEV_BROADCAST_VOLUME structure as well by adding another case label and associated deferred methods private void DeferredVolumeEvent(int eventType, DEV_BROADCAST_VOLUME dbv) and protected virtual void OnVolumeEvent(int eventType, DEV_BROADCAST_VOLUME dbv).

Other possible device types are rarely used compared to those two.

As for whether this can be backported to .Net Framework or not, it is a shame that you refer to this as a "new feature" -- I'd say it would be more fair to call it "missing feature" or even "bugfix" because ServiceBase (like so many other managed wrapper classes around native WinAPI functionality be it in .Net Framerwork or in .Net Core) was incomplete when initially written and it is still in the same sorry state in both frameworks today after so many years and versions under the belt.

What I am trying to say is -- Microsoft developers seem keen on releasing half-baked frameworks and then instead of extending them by adding missing functionality and improving orthogonality with underlying platform API they just stop development, release another half-baked framework, and expect developers to waste time and effort migrating their codebases while still having to implement and maintain hundreds of workarounds for missing or outright wrong functionality (for the latter see Environment.SetEnvironmentVariable as an example) regardless of which framework they decide to use.

[bartonjs]

Adding OnDeviceInterfaceEvent and friends is... possible, though copying/interpreting the Win32 data and passing it to a callback that is likely not even be listening seems a bit excessive. Plus, doing it for just DBT_DEVTYP_DEVICEINTERFACE feels like it'll just engender a delayed issue suggesting that we half-implemented the feature 😉. (And throwing an exception from that callback will almost certainly do bad things, so as-sketched Windows sending a DBT_DEVTYP_HANDLE event will probably crash the service.)

Exposing the raw (or nearly-raw) handler would empower any derived type to get at all of the OS events here. In fact, it might even be better to run it before the current switch:

        private int ServiceCommandCallbackEx(int command, int eventType, IntPtr eventData, IntPtr eventContext)
        {
+           try
+           {
+               if (DangerousOnServiceControl(command, eventType, eventData, eventContext))
+               {
+                   return 0;
+               }
+           }
+           catch (Exception e)
+           {
+              log it
+              return 1064; /* ERROR_EXCEPTION_IN_SERVICE */
+           }
+
            switch (command)

That way, if someone wants to handle the power notifications differently, they can. If they want to handle them both differently and the same, handle it, return false, and the existing code will continue to run. Since it's the raw Win32 info it would enable a NuGet package-based (community) extension that pretties up the device codes, and/or one that uses service triggers, or whatever. It's powerful enough to let anyone write whatever integration features on top of it... but the Dangerous and IntPtr/void* will encourage the average user to stick to overriding things like OnShutdown.

(The protected member could even behave more [LPHANDLER_FUNCTION_EX](like https://docs.microsoft.com/en-us/windows/win32/api/winsvc/nc-winsvc-lphandler_function_ex) directly and return an int with the base member returning 120 (ERROR_CALL_NOT_IMPLEMENTED)... but that's probably more complicated than it needs to be.)

As for whether this can be backported to .Net Framework or not, it is a shame that you refer to this as a "new feature"

Well, no matter what approach is taken here it'd require publishing a new targeting pack for .NET Framework (to let the compiler see whatever new method is added to ServiceBase), and that means it's beyond the "security or reliability" threshold -- we'd have to call that a new version of .NET Framework. (If it matters to you, I'm not involved in making any of these decisions, I'm just trying to set appropriate expectations.)

[levicki]

@bartonjs

Adding OnDeviceInterfaceEvent and friends is... possible, though copying/interpreting the Win32 data and passing it to a callback that is likely not even be listening seems a bit excessive.

Those device related events won't even arrive to the service callback at all unless RegisterDeviceNotification is called by the developer wanting to receive them. At most, you are adding another value comparison in a switch statement.

Plus, doing it for just DBT_DEVTYP_DEVICEINTERFACE feels like it'll just engender a delayed issue suggesting that we half-implemented the feature 😉.

I know you are joking, but it was already half-implemented since February 13, 2002, anything more than we already have, even just DBT_DEVTYP_DEVICEINTERFACE, would be considered an improvement.

And throwing an exception from that callback will almost certainly do bad things...

I am well aware of that -- sample code wasn't supposed to be taken literally 😉.

Exposing the raw (or nearly-raw) handler would empower any derived type to get at all of the OS events here. In fact, it might even be better to run it before the current switch

With that I fully agree -- I don't mind whether I get already parsed structures or a raw IntPtr in the handler as long as I can get the handler to begin with.

As I already said, I would have preferred if I could have just suggested functionality instead of an API -- I am certain (hopeful?) that Microsoft software engineers working on this area would come up with the best possible way to implement an API based on that.

...but the Dangerous and IntPtr/void* will encourage the average user to stick to overriding things like OnShutdown

I am not sure what to think about that statement.

On one hand, I understand your cautiousness because I have seen examples of Microsoft's own engineers not knowing Windows internals good enough and making mistakes when writing apps (Windows Terminal and Touch Keyboard and Handwriting Service debacle comes to mind) or framework APIs (already mentioned Environment.SetEnvironmentVariable which is unlikely to ever get fixed because "it's complicated").

On the other hand, not exposing full underlying OS functionality for services, window controls, etc, makes .Net Framework and .Net Core into frameworks which appear to be geared only toward developers writing either server or client web or database related code. I understand that's all the rage these days but that doesn't mean desktop app developers shouldn't get some love too.

As a matter of fact, if Microsoft wants to see less unsafe code in desktop apps and windows services (which are usually written in C/C++), the best way to encourage that would be to make the framework support as much native stuff as possible in a safe, platform orthogonal, well documented, manner without leaving every developer to deal with P/Invoke and marshaling on their own.

Well, no matter what approach is taken here it'd require publishing a new targeting pack for .NET Framework (to let the compiler see whatever new method is added to ServiceBase), and that means it's beyond the "security or reliability" threshold -- we'd have to call that a new version of .NET Framework.

It'd be nice if Microsoft decided to patch the SetEnvironmentVariable and release .Net Framework 4.8.1 where they could also include ServiceBase "upgrade" I proposed.

(If it matters to you, I'm not involved in making any of these decisions, I'm just trying to set appropriate expectations.)

Thanks for being honest, though I am old enough to be unsure whether my expectations can go any lower.