I have been writing a Minecraft server library in Go which, although it has grown into quite a successful project, has some issues.
One of our major issues involves handling events and the thread safety around it. To give some context: We have a *player.Player value for every player that joins the server. Users of the library can call (*player.Player).Handle(player.Handler) with a value whose type implements the player.Handler interface. One of the methods in that interface, somewhat simplified, is the following:
type Handler interface {
HandleHurt(ctx *event.Context, damage *float64)
}
The *event.Context has a Cancel() method to prevent the event from happening. The HandleHurt method is called directly before the damage is dealt to the player using the (*player.Player).Hurt(damage float64) method, which also does all sorts of other calculations. All good so far, but the problem arises when we want to ‘protect’ the Hurt() method with a sync.Mutex to avoid race conditions here. Doing this means that every player.Handler implementation that tries to additionally damage the player, or does something else that results in the player being damaged, would cause a deadlock.
Currently, we call the event handler while the relevant sync.Mutex is unlocked, and lock it directly afterwards, but this opens up race conditions because we frequently do checks before calling the handler, which are also supposed to be locked by that same mutex.
This is an issue obviously, and I was able to think of a couple of solutions, but none of the ones I could think of seem viable. One idea was to not allow cancelling events at all, so that we can call the handlers after the sync.Mutex is unlocked. This isn’t a very viable idea because some events (chat messages, for example) cannot be undone. I also considered having a look at using Mutexes that only block when trying to lock from a different goroutine than the one that currently holds the lock. This goes against all recommendations though, and with things like async preemption that isn’t even safe, so not an option either.
Do you have any ideas on how to solve this issue? I’m open to any kind of solutions or ideas.
At first, it sounds to me like the case in point for Go’s “don’t communicate by sharing; share by communicating” proverb! Is this the project? If so, where can I find an implementation of, for example, a player Handler other than the NopHandler?
I do agree this is probably where Go’s “don’t communicate by sharing; share by communicating” proverb comes into play, but I am not entirely sure how to apply this concept here. Do you have any suggestions?
That makes sense! Let me try to boil it down to its essence in a little more idiomatic Go, as the issue still arises there. Let’s say you allow users to pass a function to handle some event happening when a particular function is called:
var c = make(chan struct{})
// Let's say this function is changed by the user to allow setting a handler for an event happening:
var HandlerFunc = func() {}
func init() {
go func() {
for range c {
// For the sake of the example, pretend there isn't a race condition happening here if HandlerFunc is changed at the same time.
HandlerFunc()
}
}()
}
func CallEvent() {
c <- struct{}{}
}
Now in this very basic example, it will submit a value to the channel. In a different goroutine, it will continuously handle it. Let’s say that this goroutine will actually process the event. It then calls the handler, but there is an issue if HandlerFunc calls CallEvent() again, or some other function that would submit a value to the channel, because it would just deadlock. Buffering channels isn’t exactly the issue either, because if you call it enough times it will still deadlock and you lose the synchronous nature of submitting a value into a channel.
Is there a different pattern to get around this while maintaining thread safety?
Maybe handlers should have a different set of functions they should call if they want to trigger other events:
type Player struct {
// ...
}
func (p *Player) Hurt(ctx *event.Context, damage float64) {
// (*Player) methods should just lock and delegate to
// (*HandlerPlayer).
p.mu.Lock()
(*HandlerPlayer)(p).Hurt(ctx, damage)
p.mu.Unlock()
}
type HandlerPlayer Player
func (p *HandlerPlayer) Hurt(ctx *event.Context, damage float64) {
// Here's your actual Hurt logic that does no locking.
// If anything this Hurt function calls needs to trigger
// some other event, it should trigger it on
// HandlerPlayer, but when the whole call stack
// is done, it returns from (*Player).Hurt which will
// unlock.
}
Just need to figure out a way to prevent users from accidentally storing a Player and still deadlocking and prevent users from passing HandlerPlayer off to a different goroutine.
