State: Fix data race between ready and guild create handler

internal/moreatomic/guildid_set.go

@@ -1,51 +0,0 @@
-package moreatomic
-
-import (
-	"sync"
-
-	"github.com/diamondburned/arikawa/v2/discord"
-)
-
-type GuildIDSet struct {
-	set map[discord.GuildID]struct{}
-	mut sync.Mutex
-}
-
-// NewGuildIDSet creates a new GuildIDSet.
-func NewGuildIDSet() *GuildIDSet {
-	return &GuildIDSet{
-		set: make(map[discord.GuildID]struct{}),
-	}
-}
-
-// Add adds the passed discord.GuildID to the set.
-func (s *GuildIDSet) Add(flake discord.GuildID) {
-	s.mut.Lock()
-
-	s.set[flake] = struct{}{}
-
-	s.mut.Unlock()
-}
-
-// Contains checks whether the passed discord.GuildID is present in the set.
-func (s *GuildIDSet) Contains(flake discord.GuildID) (ok bool) {
-	s.mut.Lock()
-	defer s.mut.Unlock()
-
-	_, ok = s.set[flake]
-	return
-}
-
-// Delete deletes the passed discord.GuildID from the set and returns true if
-// the element is present. If not, Delete is a no-op and returns false.
-func (s *GuildIDSet) Delete(flake discord.GuildID) bool {
-	s.mut.Lock()
-	defer s.mut.Unlock()
-
-	if _, ok := s.set[flake]; ok {
-		delete(s.set, flake)
-		return true
-	}
-
-	return false
-}

state/event_dispatcher.go

@@ -5,57 +5,58 @@ import (
 )
 
 func (s *State) handleReady(ev *gateway.ReadyEvent) {
+	s.guildMutex.Lock()
+	defer s.guildMutex.Unlock()
+
 	for _, g := range ev.Guilds {
-		// store this so we know when we need to dispatch a belated
-		// GuildReadyEvent
-		if g.Unavailable {
-			s.unreadyGuilds.Add(g.ID)
-		} else {
-			s.Handler.Call(&GuildReadyEvent{
-				GuildCreateEvent: &g,
-			})
-		}
+		s.unreadyGuilds[g.ID] = struct{}{}
 	}
 }
 
 func (s *State) handleGuildCreate(ev *gateway.GuildCreateEvent) {
-	switch {
-	// this guild was unavailable, but has come back online
-	case s.unavailableGuilds.Delete(ev.ID):
-		s.Handler.Call(&GuildAvailableEvent{
-			GuildCreateEvent: ev,
-		})
+	s.guildMutex.Lock()
 
-	// the guild was already unavailable when connecting to the gateway
-	// we can dispatch a belated GuildReadyEvent
-	case s.unreadyGuilds.Delete(ev.ID):
-		s.Handler.Call(&GuildReadyEvent{
-			GuildCreateEvent: ev,
-		})
+	var derivedEvent interface{}
 
-	// we don't know this guild, hence we just joined it
-	default:
-		s.Handler.Call(&GuildJoinEvent{
-			GuildCreateEvent: ev,
-		})
+	// The guild was previously announced to us in the ready event, and has now
+	// become available.
+	if _, ok := s.unreadyGuilds[ev.ID]; ok {
+		delete(s.unreadyGuilds, ev.ID)
+		derivedEvent = &GuildReadyEvent{GuildCreateEvent: ev}
+
+		// The guild was previously announced as unavailable through a guild
+		// delete event, and has now become available again.
+	} else if _, ok = s.unavailableGuilds[ev.ID]; ok {
+		delete(s.unavailableGuilds, ev.ID)
+		derivedEvent = &GuildAvailableEvent{GuildCreateEvent: ev}
+
+		// We don't know this guild, hence it's new.
+	} else {
+		derivedEvent = &GuildJoinEvent{GuildCreateEvent: ev}
 	}
+
+	// Unlock here already, so we don't block the mutex if there are
+	// long-blocking synchronous handlers.
+	s.guildMutex.Unlock()
+	s.Handler.Call(derivedEvent)
 }
 
 func (s *State) handleGuildDelete(ev *gateway.GuildDeleteEvent) {
+	s.guildMutex.Lock()
+
 	// store this so we can later dispatch a GuildAvailableEvent, once the
 	// guild becomes available again.
 	if ev.Unavailable {
-		s.unavailableGuilds.Add(ev.ID)
+		s.unavailableGuilds[ev.ID] = struct{}{}
+		s.guildMutex.Unlock()
 
-		s.Handler.Call(&GuildUnavailableEvent{
-			GuildDeleteEvent: ev,
-		})
+		s.Handler.Call(&GuildUnavailableEvent{GuildDeleteEvent: ev})
 	} else {
-		// it might have been unavailable before we left
-		s.unavailableGuilds.Delete(ev.ID)
+		// Possible scenario requiring this would be leaving the guild while
+		// unavailable.
+		delete(s.unavailableGuilds, ev.ID)
+		s.guildMutex.Unlock()
 
-		s.Handler.Call(&GuildLeaveEvent{
-			GuildDeleteEvent: ev,
-		})
+		s.Handler.Call(&GuildLeaveEvent{GuildDeleteEvent: ev})
 	}
 }

state/state.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/diamondburned/arikawa/v2/discord"
 	"github.com/diamondburned/arikawa/v2/gateway"
-	"github.com/diamondburned/arikawa/v2/internal/moreatomic"
 	"github.com/diamondburned/arikawa/v2/session"
 	"github.com/diamondburned/arikawa/v2/state/store"
 	"github.com/diamondburned/arikawa/v2/state/store/defaultstore"
@@ -88,13 +87,14 @@ type State struct {
 	fewMutex    *sync.Mutex
 
 	// unavailableGuilds is a set of discord.GuildIDs of guilds that became
-	// unavailable when already connected to the gateway, i.e. sent in a
+	// unavailable after connecting to the gateway, i.e. they were sent in a
 	// GuildUnavailableEvent.
-	unavailableGuilds *moreatomic.GuildIDSet
-	// unreadyGuilds is a set of discord.GuildIDs of guilds that were
-	// unavailable when connecting to the gateway, i.e. they had Unavailable
-	// set to true during Ready.
-	unreadyGuilds *moreatomic.GuildIDSet
+	unavailableGuilds map[discord.GuildID]struct{}
+	// unreadyGuilds is a set of discord.GuildIDs of the guilds received during
+	// the Ready event. After receiving guild create events for those guilds,
+	// they will be removed.
+	unreadyGuilds map[discord.GuildID]struct{}
+	guildMutex    *sync.Mutex
 }
 
 // New creates a new state.
@@ -132,8 +132,9 @@ func NewFromSession(s *session.Session, cabinet store.Cabinet) *State {
 		readyMu:           new(sync.Mutex),
 		fewMessages:       map[discord.ChannelID]struct{}{},
 		fewMutex:          new(sync.Mutex),
-		unavailableGuilds: moreatomic.NewGuildIDSet(),
-		unreadyGuilds:     moreatomic.NewGuildIDSet(),
+		unavailableGuilds: make(map[discord.GuildID]struct{}),
+		unreadyGuilds:     make(map[discord.GuildID]struct{}),
+		guildMutex:        new(sync.Mutex),
 	}
 	state.hookSession()
 	return state