From 26f0033ebb5f8e7c204a042709e3dfa885eac868 Mon Sep 17 00:00:00 2001 From: Florian Fischer Date: Sat, 22 Jul 2023 15:15:47 +0200 Subject: fix applyTimeBonus by always using Song pointers Storing song objects in the Songs slice and using pointers to the underlying array slots breaks if the underlying array must be grown. When the array is grown the objects are copied to the new array and the pointers used in queue point to the old objects. This causes that Playlist.applyTimeBonus may not correctly apply to the most recent list of songs. This is fixed by always using Song pointers. --- wrms.go | 59 ++++++++++++++++++++++++++++++----------------------------- 1 file changed, 30 insertions(+), 29 deletions(-) (limited to 'wrms.go') diff --git a/wrms.go b/wrms.go index 02b0860..f225b2a 100644 --- a/wrms.go +++ b/wrms.go @@ -26,8 +26,8 @@ type Song struct { Downvotes map[uuid.UUID]struct{} `json:"-"` } -func NewSong(title, artist, source, uri string) Song { - return Song{ +func NewSong(title, artist, source, uri string) *Song { + return &Song{ Title: title, Artist: artist, Source: source, @@ -37,33 +37,33 @@ func NewSong(title, artist, source, uri string) Song { } } -func NewDetailedSong(title, artist, source, uri, album string, year int) Song { +func NewDetailedSong(title, artist, source, uri, album string, year int) *Song { s := NewSong(title, artist, source, uri) s.Album = album s.Year = year return s } -func NewSongFromJson(data []byte) (Song, error) { +func NewSongFromJson(data []byte) (*Song, error) { var s Song err := json.Unmarshal(data, &s) if err != nil { llog.Error("Failed to parse song data %s with %s", string(data), err) - return s, err + return nil, err } s.Upvotes = map[uuid.UUID]struct{}{} s.Downvotes = map[uuid.UUID]struct{}{} - return s, nil + return &s, nil } type Event struct { - Event string `json:"cmd"` - Id uint64 `json:"id"` - Songs []Song `json:"songs"` + Event string `json:"cmd"` + Id uint64 `json:"id"` + Songs []*Song `json:"songs"` } -func (wrms *Wrms) newEvent(event string, songs []Song) Event { +func (wrms *Wrms) newEvent(event string, songs []*Song) Event { return Event{Id: wrms.eventId.Add(1), Event: event, Songs: songs} } @@ -188,7 +188,7 @@ type Wrms struct { nextConnNr atomic.Uint64 // The rwlock must be held when using most internal state rwlock sync.RWMutex - Songs []Song + Songs []*Song queue Playlist CurrentSong atomic.Pointer[Song] Player Player @@ -234,23 +234,23 @@ func (wrms *Wrms) initConn(conn *Connection) error { } if wrms.playing { - var songs []Song + var songs []*Song if currentSong := wrms.CurrentSong.Load(); currentSong != nil { - songs = []Song{*currentSong} + songs = []*Song{currentSong} } initialCmds = append(initialCmds, wrms.newEvent("play", songs)) } - upvoted := []Song{} - downvoted := []Song{} + upvoted := []*Song{} + downvoted := []*Song{} if len(wrms.Songs) > 0 { initialCmds = append(initialCmds, wrms.newEvent("add", wrms.Songs)) for _, song := range wrms.queue.OrderedList() { if _, ok := song.Upvotes[conn.Id]; ok { - upvoted = append(upvoted, *song) + upvoted = append(upvoted, song) } else if _, ok := song.Downvotes[conn.Id]; ok { - downvoted = append(downvoted, *song) + downvoted = append(downvoted, song) } } } @@ -291,20 +291,21 @@ func (wrms *Wrms) Broadcast(ev Event) { }) } -func (wrms *Wrms) AddSong(song Song) { +func (wrms *Wrms) AddSong(song *Song) { wrms.rwlock.Lock() startPlayingAgain := wrms.playing && wrms.CurrentSong.Load() == nil - wrms.Songs = append(wrms.Songs, song) - s := &wrms.Songs[len(wrms.Songs)-1] - wrms.queue.Add(s) if wrms.Config.timeBonus != 0 { llog.Info("Apply time bonus %v", wrms.Config.timeBonus) wrms.queue.applyTimeBonus(wrms.Config.timeBonus) } - ev := wrms.newEvent("add", []Song{song}) + wrms.Songs = append(wrms.Songs, song) + s := wrms.Songs[len(wrms.Songs)-1] + wrms.queue.Add(s) + + ev := wrms.newEvent("add", []*Song{song}) wrms.rwlock.Unlock() llog.Info("Added song %s (ptr=%p) to Songs", s.Uri, s) @@ -319,7 +320,7 @@ func (wrms *Wrms) DeleteSong(songUri string) { wrms.rwlock.Lock() for i := 0; i < len(wrms.Songs); i++ { - s := &wrms.Songs[i] + s := wrms.Songs[i] if s.Uri != songUri { continue } @@ -329,7 +330,7 @@ func (wrms *Wrms) DeleteSong(songUri string) { wrms.queue.RemoveSong(s) - ev := wrms.newEvent("delete", []Song{*s}) + ev := wrms.newEvent("delete", []*Song{s}) wrms.rwlock.Unlock() wrms.Broadcast(ev) @@ -383,7 +384,7 @@ func (wrms *Wrms) _next() { cmd = "play" } - ev := wrms.newEvent(cmd, []Song{*next}) + ev := wrms.newEvent(cmd, []*Song{next}) wrms.rwlock.Unlock() wrms.Broadcast(ev) @@ -421,7 +422,7 @@ func (wrms *Wrms) PlayPause() { wrms.Player.Play(currentSong) } - ev := wrms.newEvent("play", []Song{*currentSong}) + ev := wrms.newEvent("play", []*Song{currentSong}) wrms.rwlock.Unlock() wrms.Broadcast(ev) @@ -431,7 +432,7 @@ func (wrms *Wrms) AdjustSongWeight(connId uuid.UUID, songUri string, vote string wrms.rwlock.Lock() for i := 0; i < len(wrms.Songs); i++ { - s := &wrms.Songs[i] + s := wrms.Songs[i] if s.Uri != songUri { continue } @@ -488,7 +489,7 @@ func (wrms *Wrms) AdjustSongWeight(connId uuid.UUID, songUri string, vote string } wrms.queue.Adjust(s) - ev := wrms.newEvent("update", []Song{*s}) + ev := wrms.newEvent("update", []*Song{s}) wrms.rwlock.Unlock() wrms.Broadcast(ev) @@ -496,6 +497,6 @@ func (wrms *Wrms) AdjustSongWeight(connId uuid.UUID, songUri string, vote string } } -func (wrms *Wrms) Search(pattern map[string]string) chan []Song { +func (wrms *Wrms) Search(pattern map[string]string) chan []*Song { return wrms.Player.Search(pattern) } -- cgit v1.2.3