[RFC] Remove UnifiedMessages reflection usage

[affederaffe]

This PR tries to remove the last directly existing reflection usage in SteamUnifiedMessages. The protobuf code generator was updated instead to directly handle the messaging. Obviously this is a breaking change, but, at least in my opinion, necessary in order to fully remove reflection.

[xPaw]

I like the idea. If you enable the trimming warnings, what warnings are reported? In particular handling of incoming messages.

Do note that protobuf.net is heavily reflection based, so that doesn't help us with AOT/trimming atm.

[affederaffe]

I like the idea. If you enable the trimming warnings, what warnings are reported? In particular handling of incoming messages.

Do note that protobuf.net is heavily reflection based, so that doesn't help us with AOT/trimming atm.

There seem to be no more trimming warnings aside from protobuf.net usage. I'll try to see how to avoid these too soon :tm:

[xPaw]

@yaakov-h was also looking at that, and arrived at the conclusion that switching to google.protobuf is probably the way to go.

[xPaw]

I don't however like calling CanHandleMsg for every single interface, maybe registration and a dictionary lookup would work better.

See 807fa510b9a9b8b8aa5b8d79d436acccce3d6fb7 for previous attempt at that.

[affederaffe]

I don't however like calling CanHandleMsg for every single interface, maybe registration and a dictionary lookup would work better.

See 807fa51 for previous attempt at that.

It's not really every interface, only for currently created services (until they are disposed that is). A dictionary could actually be worse for memory usage I imagine, but that'd need testing

[xPaw]

Right I see it only goes through the registered services. But it calls CanHandleMsg for every service so it does the parsing for every one of these.

The handlers list could be a dictionary of <service name, handler>, so that you can parse the method name, and directly check if a handler is registered for a particular service.

[yaakov-h]

This is a very different approach to the one I was looking at, and I do like it.

Keep in mind that consumers should be able to generate and run additional services (e.g. from updated protobufs) so some of those internal methods would need to be public.

Regarding the discussion above, perhaps each service should expose its name as a public property, then we can parse the service name once and invoke any matching handlers?

However, yes, for full trimming support and AOT we will almost certainly need to switch to Google.Protobuf. I haven't spiked AOT w/ Google just yet, but even manually hand-writing serializers and deserializers with protobuf-net.Core triggered trim and AOT warnings because it is annoyingly dependent on dynamic coding APIs.

[xPaw]

perhaps each service should expose its name as a public property

Could add service name to UnifiedService, and then have HandleMsg( method, version, packet ) and then only call HandleMsg if the service name matches.

[xPaw]

FYI this PR needs to rebase the protobufs submodule because its using an older commit.

[affederaffe]

This is a very different approach to the one I was looking at, and I do like it.

Keep in mind that consumers should be able to generate and run additional services (e.g. from updated protobufs) so some of those internal methods would need to be public.

Regarding the discussion above, perhaps each service should expose its name as a public property, then we can parse the service name once and invoke any matching handlers?

However, yes, for full trimming support and AOT we will almost certainly need to switch to Google.Protobuf. I haven't spiked AOT w/ Google just yet, but even manually hand-writing serializers and deserializers with protobuf-net.Core triggered trim and AOT warnings because it is annoyingly dependent on dynamic coding APIs.

From what I could gather, there currently is no satisfying solution:

1. Google.Protobuf with protoc ignores services completely, maybe we could modify the c# generator plugin
2. Build an AOT-compatible generator on top of protobuf-net's CommonCodeGenerator/CSharpCodeGenerator 2.1. For Google.Protobuf: would be completely AOT-compatible, although slightly cursed 2.2. For protobuf-net: less cursed, though even the protobuf-net.Core library seems to use reflection to some extend
3. (A completely home-brew solution?)

This is, however, out of scope of what I currently have time for. Per https://protobuf-net.github.io/protobuf-net/3_0, there seems to be some intention to create a source generator, but I wouldn't bet it releases any time soon

[xPaw]

Fix the build error on ci please. Also rebase the commit on master, so that the protobufs submodule does not change (to remove the unrelated changes).

[xPaw]

Breaking change for consumers:

old:

SteamUnifiedMessages.UnifiedService<ISomething> service = unifiedMessages.CreateService<ISomething>();
SteamUnifiedMessages.ServiceMethodResponse job = await service.SendMessage(x => x.DoThing(msg));
SomeResponse response = job.GetDeserializedResponse<SomeResponse>();

new:

Something service = unifiedMessages.CreateService<Something>();
SteamUnifiedMessages.ServiceMsg<SomeResponse> job = await service.DoThing(msg);
SomeResponse response = job.PacketResult;

PacketResult is an odd name, and I think ServiceMsg also needs some better name for responses.

Overall this is good because we no longer need to provide the correct type into GetDeserializedResponse.

One slight worry I have is that services directly return the service, which is an extension of UnifiedService instead of being a wrapper like UnifiedService<Something>

[xPaw]

I just noticed that SendNotification is not being generated, for example RemoteClient.NotifyOnline#1. I think we can do it when response type is NoResponse.

[affederaffe]

One slight worry I have is that services directly return the service, which is an extension of UnifiedService instead of being a wrapper like UnifiedService<Something>

I think this would be rather complicated to achieve since we'd need some form of mapping between the interface and implementation. Since there will be another breaking change when switching to Google.Protobuf anyway and I don't think the public api of the generated classes will change until then, this is good enough.

[xPaw]

013_UnifiedMessages sample needs updating.

Shall we keep ServiceMethodResponse name for ServiceMsg? ServiceMethodResponse.Result will remain unchanged then.

[LossyDragon]

I do have a question about how this PR will work on Unified Service methods that we wouldn't necessarily post a request first.

Such as FriendMessagesClient.IncomingMessage (NotifyAckMessageEcho, Etc). When the client is logged in using the NewSteamChat, what would I subscribe to in order to handle any unified service methods that are sent to the client?

[xPaw]

CreateService<FriendMessagesClient> calling would do that (but you have to keep a ref so it doesn't dispose), maybe we need a create-and-forget register method that doesn't return a disposable? ~~but then that also requires an unregister call.~~

[LossyDragon]

I removed my previous post as I was wrong about how the proposed implementation works and as I was writing up a new response, I saw xPaw pointed out the issues to be addressed. Thanks 😄

This brings up another question I have. Since ServiceMethodResponse and ServiceMethod(Notification), are technically consumers right now. Would the ServiceMethodResponse<T> be eliminated as a consumer; since the example shows we await for that resp, and ServiceMethodNotification stays as one?

[xPaw]

No it should stay if people want to directly handle incoming messages (I am not entirely sure if Steam can send you a response not a notification without asking for one -- but no reason to break this), this still matches the callbackmanager / asyncjobs design.

[xPaw]

@affederaffe did you see my comment in regards to the incoming messages/notifications?

[affederaffe]

@affederaffe did you see my comment in regards to the incoming messages/notifications?

Yes, sorry, time is currently sparse. I'll implement a few ideas and see what works best.

[xPaw]

Just a thought I had, calling CreateService multiple times like in d9f6b1b1d38d51823ca77f8e51648cafb2ce264a might be problematic, as it returns a disposable service, and the handler is being added by the name.

Wouldn't it break if you called CreateService multiple times? Maybe to simplify services should be non disposable and it should return an existing one (handlers.GetOrAdd basically).

[JustArchi]

This piece of code is not working (callback function handler not being called despite event arriving):

CallbackManager.Subscribe<SteamUnifiedMessages.ServiceMethodNotification<CChatRoom_IncomingChatMessage_Notification>>(OnIncomingChatMessage);

CallbackManager.Subscribe<SteamUnifiedMessages.ServiceMethodNotification<CFriendMessages_IncomingMessage_Notification>>(OnIncomingMessage);

Previously I had this and it worked properly:

CallbackManager.Subscribe<SteamUnifiedMessages.ServiceMethodNotification>(OnServiceMethod);

private async void OnServiceMethod(SteamUnifiedMessages.ServiceMethodNotification notification) {
		ArgumentNullException.ThrowIfNull(notification);

		switch (notification.MethodName) {
			case "ChatRoomClient.NotifyIncomingChatMessage#1":
				await OnIncomingChatMessage((CChatRoom_IncomingChatMessage_Notification) notification.Body).ConfigureAwait(false);

				break;
			case "FriendMessagesClient.IncomingMessage#1":
				await OnIncomingMessage((CFriendMessages_IncomingMessage_Notification) notification.Body).ConfigureAwait(false);

				break;
		}
	}

My experimental code lies in https://github.com/JustArchiNET/ArchiSteamFarm/tree/unified-experiments, the diff with main is https://github.com/JustArchiNET/ArchiSteamFarm/compare/main...unified-experiments. You can also get artifacts for testing in https://github.com/JustArchiNET/ArchiSteamFarm/actions/workflows/publish.yml?query=branch%3Aunified-experiments. If I did something wrong, let me know and I can give it another try.

[affederaffe]

Just a thought I had, calling CreateService multiple times like in d9f6b1b might be problematic, as it returns a disposable service, and the handler is being added by the name.

Wouldn't it break if you called CreateService multiple times? Maybe to simplify services should be non disposable and it should return an existing one (handlers.GetOrAdd basically).

I agree that using a single instance is probably the way to go, but it should still be disposable in order to be removed from _handlers.

[xPaw]

but it should still be disposable

This could be achieved by adding a separate call to remove it. I think that's more explicit than leaving it disposable (because if you call CreateService for some temporary call, disposing it would be dispose it for everything).

[affederaffe]

This piece of code is not working (callback function handler not being called despite event arriving):

CallbackManager.Subscribe<SteamUnifiedMessages.ServiceMethodNotification<CChatRoom_IncomingChatMessage_Notification>>(OnIncomingChatMessage);

CallbackManager.Subscribe<SteamUnifiedMessages.ServiceMethodNotification<CFriendMessages_IncomingMessage_Notification>>(OnIncomingMessage);

Previously I had this and it worked properly:

CallbackManager.Subscribe<SteamUnifiedMessages.ServiceMethodNotification>(OnServiceMethod);

private async void OnServiceMethod(SteamUnifiedMessages.ServiceMethodNotification notification) {
		ArgumentNullException.ThrowIfNull(notification);

		switch (notification.MethodName) {
			case "ChatRoomClient.NotifyIncomingChatMessage#1":
				await OnIncomingChatMessage((CChatRoom_IncomingChatMessage_Notification) notification.Body).ConfigureAwait(false);

				break;
			case "FriendMessagesClient.IncomingMessage#1":
				await OnIncomingMessage((CFriendMessages_IncomingMessage_Notification) notification.Body).ConfigureAwait(false);

				break;
		}
	}

My experimental code lies in https://github.com/JustArchiNET/ArchiSteamFarm/tree/unified-experiments, the diff with main is JustArchiNET/[email protected]. You can also get artifacts for testing in https://github.com/JustArchiNET/ArchiSteamFarm/actions/workflows/publish.yml?query=branch%3Aunified-experiments. If I did something wrong, let me know and I can give it another try.

You need to create the service the notification is coming from first with SteamUnifiedMessages.CreateService as it is used to map the notification method name to the result type internally.

[xPaw]

You need to create the service the notification is coming from first

He does in ArchiHandler.

[affederaffe]

You need to create the service the notification is coming from first

He does in ArchiHandler.

ChatRoomClient and FriendMessagesClient are missing, which is why the message cannot be dispatched; there's no handler registered for it.

[xPaw]

My bad, missed the "Client" part.

This is outside of the scope of this PR, maybe a future improvement: Subscribing to incoming notifications could be done through CreateService so that you couldn't mess this up.

EDIT: Can we add the sample of receiving a notification like this to the unified messages sample? So that it is documented that you need create a service to receive stuff. FriendMessagesClient.IncomingMessage should be easily testable. Sorry for a lot of back-and-forth on this PR, just want to minimize any pain for consumers.

[xPaw]

One solution I've come up to solve the handler not being registered is this:

1. mgr.Subscribe<ChatRoomClient, CChatRoom_IncomingChatMessage_Notification>(func)
2. ChatRoomClient.Subscribe<CChatRoom_IncomingChatMessage_Notification>(mgr, func)

The first option makes sense because the callbacks still go through the manager, and probably preferrable to keep the logic in same place. It probably needs a separate method name, like SubscribeServiceMethodResponse and ServiceMethodNotification. The second option makes sense because you are subscribing on a specific service (but you still have to give it a manager).

However this still doesn't prevent users from shooting themselves in the foot because you still can register <WrongService, DifferentMethod>, as that would have to be validated somehow (generate another method to check? but that seems overkill, maybe at that point its not our problem anymore).

And mgr.Subscribe would have to disallow subscribing directly to ServiceMethodNotification<T>.

Thoughts? Any different ideas?

P.S. See my comment above about adding a sample for receiving notifications.