Pryaxis / orion-core

The next generation Terraria Server API.
https://pryaxis.github.io/orion-core
GNU General Public License v3.0
31 stars 12 forks source link

Orion Framework: Internal Message Queue #48

Closed Ijwu closed 4 years ago

Ijwu commented 6 years ago

Event handling, in its current form, relies heavily on the C# event object. This is seriously limited in that services may not define prioritization or handling order.

An internal message queue would allow for all services to write events to the queue and allow all other services to receive and respond to messages in a pub-sub pattern.

Ijwu commented 6 years ago

image

bartico6 commented 6 years ago

Essentially the problem is that currently only the networking classes are publishers, which means that plugins that emulate any sort of behavior that would normally come from networking classes are essentially unfeasible as you have to hack your way into the networking (if even possible) rather than simply fire an event.

I think the way Bukkit works, a similar solution to which is being proposed here, (you subscribe to events (that can come from networking or from other plugins) rather than hook networking) is good because it both enables serverside emulation and allows plugins to easily communicate between each other (a plugin can fire CustomPlayerAbilityEvent and have another addon handle it and grant buffs or currency for it). 😄

tylerjwatson commented 6 years ago

My original design of Orion handled this kind of thing, but not in any specific form. In my designs, I simply used the .NET event mechanism for attaching delegates onto event handlers.

I suppose it depends on what you mean by limited. Events are limited in and of themselves, but I suppose I am asking what is the actual problem we are trying to solve here?

Cheers, T.

bartico6 commented 6 years ago
Ijwu commented 6 years ago

Question for you @tylerjwatson: If a plugin needs to revert or cancel and event, how does it go around doing that without causing issues for others? From what I've seen, it's a legitimate use case for a plugin, such as region protection, to throw out an event with intent to ignore it. If it doesn't handle it first then another, less important or critical, plugin could break the chain first causing a security issue.

QuiCM commented 6 years ago

With that being said, 'handling' an event does not currently break the chain. The onus falls upon other plugins to check the handled state of the event which is, in my opinion, unintuitive

tylerjwatson commented 6 years ago

The onus falls upon other plugins to check the handled state of the event which is, in my opinion, unintuitive

Agreed, I think we can improve that design a lot by checking the flag at each stage.

bartico6 commented 6 years ago

If tShock had its handlers running before plugins (due to, say, registering with a higher priority) then plugins should have a way to 'un-cancel' the event, same goes for other plugins' actions being "reversed" by another plugin further down the chain. Here is a viable example, yet again, from Bukkit: Example

This event can be marked as disallowed before the first plugin sees it (by the server) - this is good because it allows to (rather than stop the server from doing anything completely) override only certain actions made by it. In this case, if the stock server thought the server is full (the plugin has its own max-players override) but my plugin didn't - the event would be reset to the stock, "allowed" state.

As a compromise, plugins that don't plan to do hardcore event reversing stuff should have a flag when registering their event - "no-cancelled-events" which would not send any events that are marked as handled to their callbacks.

kevzhao2 commented 5 years ago

In the reboot branch, I now have a HookHandlerCollection<TArgs> class which should handle this properly. You can annotate hook handlers with priorities, and the class is immutable and hence thread-safe.

kevzhao2 commented 4 years ago

Closing as these concerns will no longer be relevant due to the new event API.