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