[RFC] Remove UnifiedMessages reflection usage #1433

affederaffe · 2024-09-18T19:22:18Z

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 · 2024-09-18T19:45:29Z

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.

Resources/ProtobufGen/ProtobufGen/SteamKitCSharpCodeGenerator.cs

affederaffe · 2024-09-18T19:53:15Z

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 ™️

xPaw · 2024-09-18T19:54:07Z

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

xPaw · 2024-09-18T20:05:45Z

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.

affederaffe · 2024-09-18T20:24:01Z

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 · 2024-09-18T20:35:27Z

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 · 2024-09-19T00:42:28Z

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 · 2024-09-19T08:18:48Z

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 · 2024-09-20T08:13:55Z

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

affederaffe · 2024-09-25T22:12:41Z

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:

Google.Protobuf with protoc ignores services completely, maybe we could modify the c# generator plugin
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
(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

Resources/ProtobufGen/ProtobufGen/SteamKitCSharpCodeGenerator.cs

SteamKit2/SteamKit2/Steam/Handlers/SteamUnifiedMessages/SteamUnifiedMessages.cs

xPaw · 2024-09-26T10:32:19Z

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 · 2024-09-26T10:41:12Z

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>

SteamKit2/SteamKit2/Steam/Handlers/SteamUnifiedMessages/SteamUnifiedMessages.cs

xPaw · 2024-09-26T11:12:26Z

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 · 2024-09-27T13:22:02Z

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.

SteamKit2/SteamKit2/Base/Generated/SteamMsgRemoteClient.cs

# Conflicts: # Resources/Protobufs # SteamKit2/SteamKit2/Base/Generated/SteamMsgAuth.cs # SteamKit2/SteamKit2/Base/Generated/SteamMsgStoreBrowse.cs # SteamKit2/SteamKit2/Base/Generated/WebUI/SteamMsgAccountCart.cs # SteamKit2/SteamKit2/Base/Generated/WebUI/SteamMsgCommon.cs

xPaw · 2024-10-13T17:51:31Z

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).

JustArchi · 2024-10-13T18:12:52Z

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/ArchiSteamFarm@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 · 2024-10-13T19:16:49Z

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 · 2024-10-13T19:18:29Z

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 · 2024-10-13T19:19:41Z

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.

…Service method

xPaw · 2024-10-13T19:29:56Z

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

He does in ArchiHandler.

affederaffe · 2024-10-13T19:35:37Z

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 · 2024-10-13T19:41:04Z

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 · 2024-10-14T18:06:55Z

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

mgr.Subscribe<ChatRoomClient, CChatRoom_IncomingChatMessage_Notification>(func)
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.

Thoughts? Any different ideas?

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

Resources/ProtobufGen/ProtobufGen/SteamKitCSharpCodeGenerator.cs

JustArchi · 2024-10-14T18:09:38Z

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.

Thanks, I can confirm that it works after adding required clients.

The only outstanding issue I see with this PR is that right now, it's possible to register for callback but skip registration of the service.

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

As @xPaw suggested above, any of those two would fix it. If you asked me, I like first one slightly more.

affederaffe · 2024-10-15T21:42:05Z

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.

Thoughts? Any different ideas?

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

The approach to subscribe to the manager directly seems more in line with what users currently do and therefore what is probably expected.
Additionally mapping types to method return types seems overkill to me, imo just documenting the behaviour is enough.
Disallowing ServiceMethodNotification<T> or ServiceMethodResponse<T> should be doable with type constraints.

I'll try to implement this soon and add/update the samples

voided

This looks really good!

I agree with having that one-line approach of subscribing to notifications via the callback manager - infinitely less foot-guns than needing to remember to create a service.

I'll defer to @xPaw for the final approval, feel free to consider my comments nit-picky. :)

voided · 2024-10-15T22:44:12Z

SteamKit2/SteamKit2/Steam/Handlers/SteamUnifiedMessages/SteamUnifiedMessages.cs

    /// </summary>
    public partial class SteamUnifiedMessages : ClientMsgHandler
    {
+        private readonly ConcurrentDictionary<string, object> _handlers = [ ];


Can this be ConcurrentDictionary<string, IUnifiedService> to avoid the various casts/as to TService?

Seems like all the type constraints are always IUnifiedService.

voided · 2024-10-15T22:44:36Z

SteamKit2/SteamKit2/Steam/Handlers/SteamUnifiedMessages/SteamUnifiedMessages.cs

-                        break;
-                }
+            return ( _handlers.GetOrAdd( TService.ServiceName, static ( _, unifiedMessages ) =>
+                new TService { UnifiedMessages = unifiedMessages }, this ) as TService )!;


Could cast directly to TService rather than as - that should avoid the extra null assertion operator (or whatever ! is called).

voided · 2024-10-15T22:52:05Z

SteamKit2/SteamKit2/Base/Generated/SteamMsgWorkshop.cs

+        public static string ServiceName { get; } = "Workshop";
+
+        /// <inheritdoc />
+        public SteamUnifiedMessages UnifiedMessages { get; init; }


Leaving this comment for this service, but this applies to how the generator created all these.

Feels like this should be internal rather than public. A consumer could either interact with the service methods directly, or grab the handler from the SteamClient.

There's lots of pre-existing undefined behavior that I unfortunately designed into the callback manager, but it makes me think about (edge) cases like RemoveServiceing a service but then still holding on to it for this UnifiedMessages property.

What if a consumer wants to generate and use their own service?

Good point. I think an explicit interface implementation can solve for that: https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/interfaces/explicit-interface-implementation

It would hide access to the property on the implementation types and only allow access if you cast to a IUnifiedService, which the SteamUnifiedMessages handler can easily do.

A consumer could end up doing the same, but it would at least make it more unlikely.

Yeah it would be good to get rid of this property from the generated classes, and only leave it on the parent.

voided · 2024-10-15T22:59:45Z

SteamKit2/SteamKit2/Steam/SteamClient/CallbackMgr/CallbackMgr.cs

+        /// </summary>
+        /// <param name="callbackFunc">The function to invoke with the callback.</param>
+        /// <returns>An <see cref="IDisposable"/>. Disposing of the return value will unsubscribe the <paramref name="callbackFunc"/>.</returns>
+        public IDisposable SubscribeNotifications<TService, TNotification>( Action<SteamUnifiedMessages.ServiceMethodNotification<TNotification>> callbackFunc )


SubscribeServiceNotification perhaps?

JustArchi

Works for me, summary of all changes I did: JustArchiNET/ArchiSteamFarm@main...unified-experiments

xPaw · 2024-10-21T08:15:25Z

@voided's comments were caused by making servicename a static, I reverted that. Squashed all the commits with minor cleanup and merged in 2ba0a55

Thanks!

affederaffe added 3 commits September 18, 2024 21:12

Remove ICallbackMsg, inherit CallbackMsg directly

8eb12e3

Update protobuf code generator to remove reflection usage

c2644e6

Regenerate protobufs

fcbd49c

xPaw reviewed Sep 18, 2024

View reviewed changes

Resources/ProtobufGen/ProtobufGen/SteamKitCSharpCodeGenerator.cs Outdated Show resolved Hide resolved

affederaffe added 3 commits September 26, 2024 00:27

Only call correct handler

edc8104

Update protobufs

83424d2

Update protobufs

081d099

xPaw reviewed Sep 26, 2024

View reviewed changes

Resources/ProtobufGen/ProtobufGen/SteamKitCSharpCodeGenerator.cs Outdated Show resolved Hide resolved

xPaw reviewed Sep 26, 2024

View reviewed changes

SteamKit2/SteamKit2/Steam/Handlers/SteamUnifiedMessages/SteamUnifiedMessages.cs Outdated Show resolved Hide resolved

affederaffe added 4 commits September 26, 2024 11:53

Fix formatting

7540f2c

Add comments + make methods public

ff78cef

Adjust to modifier change

0fc672f

Regenerate protobufs

7698f76

xPaw reviewed Sep 26, 2024

View reviewed changes

SteamKit2/SteamKit2/Steam/Handlers/SteamUnifiedMessages/SteamUnifiedMessages.cs Outdated Show resolved Hide resolved

xPaw mentioned this pull request Sep 26, 2024

Allow UnifiedMessages to be transported over webapi #1438

Open

affederaffe commented Sep 27, 2024

View reviewed changes

SteamKit2/SteamKit2/Base/Generated/SteamMsgRemoteClient.cs Outdated Show resolved Hide resolved

affederaffe added 3 commits October 13, 2024 11:51

Update Protobufs

1eb8f6b

Use new UnifiedMessages service for SteamContent

d9f6b1b

affederaffe added 2 commits October 13, 2024 21:23

Use single instance for UnifiedMessage services + use explicit Remove…

c2fb158

…Service method

Remove using statements for no longer disposable services

2caae35

affederaffe added 4 commits October 13, 2024 21:56

Use static abstract property for ServiceName in IUnifiedService

84c08e1

Regenerate protobufs

a887aca

Regenerate protobufs

452aabb

Merge remote-tracking branch 'origin/master'

55f3a1e

xPaw reviewed Oct 14, 2024

View reviewed changes

Resources/ProtobufGen/ProtobufGen/SteamKitCSharpCodeGenerator.cs Outdated Show resolved Hide resolved

Resources/ProtobufGen/ProtobufGen/SteamKitCSharpCodeGenerator.cs Outdated Show resolved Hide resolved

Fix code style

e0a57c8

affederaffe added 2 commits October 16, 2024 00:18

Add specific Subscibe* methods for notifications and service responses

fcb6057

Add notifications section to UnifiedMessages sample

88a44c4

voided reviewed Oct 15, 2024

View reviewed changes

xPaw mentioned this pull request Oct 16, 2024

Remove ICallbackMsg interface #1453

Closed

JustArchi approved these changes Oct 17, 2024

View reviewed changes

xPaw closed this in 2ba0a55 Oct 21, 2024

[RFC] Remove UnifiedMessages reflection usage #1433

[RFC] Remove UnifiedMessages reflection usage #1433

Conversation

affederaffe commented Sep 18, 2024

xPaw commented Sep 18, 2024 • edited Loading

affederaffe commented Sep 18, 2024

xPaw commented Sep 18, 2024

xPaw commented Sep 18, 2024 • edited Loading

affederaffe commented Sep 18, 2024

xPaw commented Sep 18, 2024

yaakov-h commented Sep 19, 2024

xPaw commented Sep 19, 2024

xPaw commented Sep 20, 2024

affederaffe commented Sep 25, 2024

xPaw commented Sep 26, 2024 • edited Loading

xPaw commented Sep 26, 2024 • edited Loading

xPaw commented Sep 26, 2024

affederaffe commented Sep 27, 2024

xPaw commented Oct 13, 2024 • edited Loading

JustArchi commented Oct 13, 2024 • edited Loading

affederaffe commented Oct 13, 2024

xPaw commented Oct 13, 2024

affederaffe commented Oct 13, 2024

xPaw commented Oct 13, 2024

affederaffe commented Oct 13, 2024

xPaw commented Oct 13, 2024 • edited Loading

xPaw commented Oct 14, 2024 • edited Loading

JustArchi commented Oct 14, 2024

affederaffe commented Oct 15, 2024

voided left a comment

Choose a reason for hiding this comment

voided Oct 15, 2024

Choose a reason for hiding this comment

voided Oct 15, 2024

Choose a reason for hiding this comment

voided Oct 15, 2024

Choose a reason for hiding this comment

xPaw Oct 16, 2024

Choose a reason for hiding this comment

voided Oct 16, 2024

Choose a reason for hiding this comment

xPaw Oct 16, 2024

Choose a reason for hiding this comment

voided Oct 15, 2024

Choose a reason for hiding this comment

JustArchi left a comment

Choose a reason for hiding this comment

xPaw commented Oct 21, 2024

xPaw commented Sep 18, 2024 •

edited

Loading

xPaw commented Sep 18, 2024 •

edited

Loading

xPaw commented Sep 26, 2024 •

edited

Loading

xPaw commented Sep 26, 2024 •

edited

Loading

xPaw commented Oct 13, 2024 •

edited

Loading

JustArchi commented Oct 13, 2024 •

edited

Loading

xPaw commented Oct 13, 2024 •

edited

Loading

xPaw commented Oct 14, 2024 •

edited

Loading