CallbackManager rewrite discussion

[JustArchi]

Issue #691 highlighted something that I'm not that concerned about as advanced SK2 consumer, but something that could definitely be improved especially for newcomers, but also more advanced usages.

I'm looking at CallbackManager here and the expectation from the consumer to handle the callbacks himself. Personally I believe that this could be SK2 built-in, as I don't really see a huge need of putting the burden on the consumer, especially considering I don't see any huge advantage of the consumer being able to handle it himself to begin with.

Apart from Subscribe() calls done in the constructor, which could simply be provided by SteamClient itself, I'm making use of it only in a handling loop, which eradicated out of my ASF-specific redundant bits, would look like this:

private void HandleCallbacks() {
	TimeSpan timeSpan = TimeSpan.FromMilliseconds(CallbackSleep);

	while (KeepRunning || SteamClient.IsConnected) {
		CallbackManager.RunWaitAllCallbacks(timeSpan);
	}
}

I believe that SK2 could include logic similar to above on its own. We could have something like SteamClient.HandleCallbacks bool property, with default either of false or true, which would work like KeepRunning in my code above. Simply, callback manager loop should work as long as either this property is true (to handle initial login), or steam client is connected (to handle eventual messages). Since Steam client needs to answer heartbeats in order to stay alive, you can't possibly want SteamClient running without any callbacks handling, at least I think so, correct me if I'm wrong.

Alternatively, a bit more advanced logic would be to make the above HandleCallbacks private and transparent to the consumer, because you could also argue that it's enough to launch it when user executes Connect(), and disable it when he gets OnDisconnected(), assuming SK2 doesn't retry on its own. You'd still need similar variable to achieve the goal, but the consumer wouldn't need to do anything at all anymore. Probably you already have something similar in place that could be used for the above (maybe IsConnected itself with a bit of launch/kill logic), so even a new variable wouldn't be required.

The rewrite idea could get along with #425 and make the solution more efficient, as it's now SK2 implementation detail how exactly callbacks should be handled. We could decide between a Timer, a Task running on a threadpool or a dedicated Thread, depending on what is the most appropriate for the implementation you decide to stick with.

Thoughts? I was trying to find any use case for consumer handling callbacks himself, and honestly I couldn't think of anything that wouldn't be possible with simple SteamClient.HandleCallbacks set to true or false. Any specific logic related to callbacks and messages could be achieved with appropriate ClientHandler instead, if needed.

[yaakov-h]

It's been mentioned some time years ago on IRC that there's definitely room for one or more opinionated wrapper libraries in this space.

There aren't really any better options for this sort of event-driven / message-driven system.

We can't invoke callbacks from the networking thread, because that has a high potential to slow things down, cause deadlocks, crash, etc.

We can't dispatch callbacks to the thread pool, because there could then be out-of-order processing or unwanted parallel processing.

We also don't want to create our own thread for each SteamClient, because we could very quickly explode with too many threads in scenarios like steamstat.us with a high number of clients.

In UI scenarios like Windows Forms or WPF apps, a consumer might want to process messages on the UI thread or Dispatcher thread, so that they can reliably update the UI to respond to these callbacks. In a WPF app you could even use something like a DispatcherTimer so that SteamKit is involved in the dispatcher queue. This would require no new thread and also wouldn't block the UI thread.

I'm not sure what you mean with regard to heartbeats because SteamKit handles heartbeats itself with an internal timer. We can do that because it doesn't require any interaction with the library consumer.

[JustArchi]

I'm not sure what you mean with regard to heartbeats because SteamKit handles heartbeats itself with an internal timer. We can do that because it doesn't require any interaction with the library consumer.

Does it work without callback manager processing messages? If yes then it could be one possibility for running Steam client without callback manager at all, but this leaves a question why the same mechanism couldn't be used for all messages and not just that one specifically?

We can't invoke callbacks from the networking thread, because that has a high potential to slow things down, cause deadlocks, crash, etc.

We can't dispatch callbacks to the thread pool, because there could then be out-of-order processing or unwanted parallel processing.

We also don't want to create our own thread for each SteamClient, because we could very quickly explode with too many threads in scenarios like steamstat.us with a high number of clients.

Yet the consumer has to decide upon something and he's in no better position than the library. Sure, UI apps offer some more room in terms of implementation, but nothing that couldn't be handled with events and callbacks in the first place. Even if you decide that UI apps could want to handle callback manager themselves, it's still a good idea to provide sane default implementation that they can use if they don't require such more advanced logic, and that assumes you can't do that better right away.

I have one concept for now that would satisfy at least majority of concerns above:

• Maintain a FIFO queue for all Steam Clients in the process, there will be one callback manager for everyone
• FIFO queue needs to be concurrent, something like ConcurrentStack<>
• Networking thread adds message into the FIFO queue, and depending upon the implementation, optionally also wakes up the thread responsible for going with it (async sleep on something like *ResetEvent comes to mind).
• Once thread wakes up, it processes all the messages in the queue, in-order, handling the callbacks to appropriate subscribers. It's non-locking mechanism since network threads can keep adding things all the time.
• Once the queue is empty, the thread either waits in a loop, or goes to sleep until new messages arrive (once again depending upon final implementation).

The only drawback I see is the fact that we limit this callbacks handling to a single thread, creating potential bottleneck, but we could always have some configurable switch specifying whether callback manager should be used to begin with, since I'd totally experiment with parallel out-of-order processing, which can be very efficient approach to the problem as long as application doesn't depend on any particular order of events, and personally I believe that a good app shouldn't. In this case networking thread would schedule callbacks to the threadpool right away, entirely avoiding the problem, while benefiting use cases that do not need FIFO queue and callbacks order guarantee to begin with. And the best part is how it'd also auto-scale itself depending on the workload, as opposed to single thread bottleneck. I'd definitely at least try to make it work with ASF, as it's one of the most efficient and clean solutions at least for my own use case.

[yaakov-h]

Does it work without callback manager processing messages? If yes then it could be one possibility for running Steam client without callback manager at all, but this leaves a question why the same mechanism couldn't be used for all messages and not just that one specifically?

Yes it does, but I don't see how the same mechanism could be used for other messages.

On login, the CMs tells us how often we have to heartbeat. We setup a timer. Whenever that timer fires, we send the heartbeat "hey I'm still here" message.

How would that work for something like MachineAuth or PersonaStateChange?

Maintain a FIFO queue for all Steam Clients in the process, there will be one callback manager for everyone.

I would very much be in favour of this option, of having the ability to have one callback pump for N steam clients. ✨

FIFO queue needs to be concurrent, something like ConcurrentStack<>

How about ConcurrentQueue<T>? 😉

Networking thread adds message into the FIFO queue, and depending upon the implementation, optionally also wakes up the thread responsible for going with it (async sleep on something like *ResetEvent comes to mind).

We already do this. The networking thread / ClientMsgHandler posts the callback and then the waiting thread picks it up.

Once thread wakes up, it processes all the messages in the queue, in-order, handling the callbacks to appropriate subscribers. It's non-locking mechanism since network threads can keep adding things all the time.

That is literally how CallbackManager works.

Once the queue is empty, the thread either waits in a loop, or goes to sleep until new messages arrive (once again depending upon final implementation).

This bit is currently left up to the consumer-defined callback pump.

[JustArchi]

On login, the CMs tells us how often we have to heartbeat. We setup a timer. Whenever that timer fires, we send the heartbeat "hey I'm still here" message.

Right, I thought of the fact that you already handle that message, so you need to know what message it is, what you should deserialize it as and what to do with it, so pretty much what callbacks are used for, except they add a tiny bit of extra logic in regards to wrapping protobuf message and making it easier to consume.

I would very much be in favour of this option, of having the ability to have one callback pump for N steam clients. ✨

It sounds like good enough approach for majority of use cases. Right now it's not possible to achieve that, so it'd be one good thing to consider implementing regarding this, and once we manage to have an implementation like that, there is no real reason not to add, maybe optional, handling for consumer as well, in form of a Thread or something else. And we don't even need to resign from existing sort of things, barely SteamConfiguration.HandleCallbackManager = true 🙂.

I'd still vouch for an option to skip callback manager entirely and shift the logic right into networking thread, which would immediately schedule what CallbackManager does, as a Task on some threadpool thread. Of course that would be optional and the consumer would need to opt-in, but it could vastly improve performance, reduce overhead and help the flow if the consumer doesn't need FIFO guarantee in callbacks firing.

[yaakov-h]

as a Task on some threadpool thread

and then we get into the horribly un-fun world of exception handling. 😭

[yaakov-h]

I'm thinking that we could probably simplify this somewhat by having each SteamClient hold on to a BlockingCollection<T> that wraps a ConcurrentQueue<T>, and have CallbackManager be able to listen to multiple steamclients through BlockingCollection<T>.TakeFromAny.