From f8ee6db72a015f33b41879d1ba253eeff8884bfd Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 19 Jun 2021 20:56:56 -0400 Subject: [PATCH] New implementation of NowPlaying --- cmd/wire_gen.go | 7 +- core/nowplaying.go | 106 ------------------------- core/nowplaying_test.go | 61 -------------- core/scrobbler/scrobbler.go | 73 +++++++++++++++++ core/wire_providers.go | 3 +- git/pre-commit | 2 +- server/subsonic/album_lists.go | 14 ++-- server/subsonic/api.go | 5 +- server/subsonic/media_annotation.go | 23 +++--- server/subsonic/responses/responses.go | 6 +- server/subsonic/wire_gen.go | 21 +++-- server/subsonic/wire_injectors.go | 14 +++- utils/singleton/singleton.go | 48 +++++++++++ utils/singleton/singleton_test.go | 53 +++++++++++++ 14 files changed, 233 insertions(+), 203 deletions(-) delete mode 100644 core/nowplaying.go delete mode 100644 core/nowplaying_test.go create mode 100644 core/scrobbler/scrobbler.go create mode 100644 utils/singleton/singleton.go create mode 100644 utils/singleton/singleton_test.go diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 6a8d4bc6..a35e0d4a 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -6,10 +6,9 @@ package cmd import ( - "sync" - "github.com/google/wire" "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/core/transcoder" "github.com/navidrome/navidrome/persistence" "github.com/navidrome/navidrome/scanner" @@ -18,6 +17,7 @@ import ( "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/server/nativeapi" "github.com/navidrome/navidrome/server/subsonic" + "sync" ) // Injectors from wire_injectors.go: @@ -48,7 +48,8 @@ func CreateSubsonicAPIRouter() *subsonic.Router { externalMetadata := core.NewExternalMetadata(dataStore) scanner := GetScanner() broker := GetBroker() - router := subsonic.New(dataStore, artwork, mediaStreamer, archiver, players, externalMetadata, scanner, broker) + scrobblerScrobbler := scrobbler.New(dataStore) + router := subsonic.New(dataStore, artwork, mediaStreamer, archiver, players, externalMetadata, scanner, broker, scrobblerScrobbler) return router } diff --git a/core/nowplaying.go b/core/nowplaying.go deleted file mode 100644 index 7431505d..00000000 --- a/core/nowplaying.go +++ /dev/null @@ -1,106 +0,0 @@ -package core - -import ( - "container/list" - "sync" - "time" -) - -const NowPlayingExpire = 60 * time.Minute - -type NowPlayingInfo struct { - TrackID string - Start time.Time - Username string - PlayerId int - PlayerName string -} - -// This repo must have the semantics of a FIFO queue, for each playerId -type NowPlaying interface { - // Insert at the head of the queue - Enqueue(*NowPlayingInfo) error - - // Returns all heads from all playerIds - GetAll() ([]*NowPlayingInfo, error) -} - -var playerMap = sync.Map{} - -type nowPlayingRepository struct{} - -func NewNowPlayingRepository() NowPlaying { - r := &nowPlayingRepository{} - return r -} - -func (r *nowPlayingRepository) Enqueue(info *NowPlayingInfo) error { - l := r.getList(info.PlayerId) - l.PushFront(info) - return nil -} - -func (r *nowPlayingRepository) GetAll() ([]*NowPlayingInfo, error) { - var all []*NowPlayingInfo - playerMap.Range(func(playerId, l interface{}) bool { - ll := l.(*list.List) - e := checkExpired(ll, ll.Front) - if e != nil { - all = append(all, e.Value.(*NowPlayingInfo)) - } - return true - }) - return all, nil -} - -func (r *nowPlayingRepository) getList(id int) *list.List { - l, _ := playerMap.LoadOrStore(id, list.New()) - return l.(*list.List) -} - -func (r *nowPlayingRepository) dequeue(playerId int) (*NowPlayingInfo, error) { - l := r.getList(playerId) - e := checkExpired(l, l.Back) - if e == nil { - return nil, nil - } - l.Remove(e) - return e.Value.(*NowPlayingInfo), nil -} - -func (r *nowPlayingRepository) head(playerId int) (*NowPlayingInfo, error) { - l := r.getList(playerId) - e := checkExpired(l, l.Front) - if e == nil { - return nil, nil - } - return e.Value.(*NowPlayingInfo), nil -} - -func (r *nowPlayingRepository) tail(playerId int) (*NowPlayingInfo, error) { - l := r.getList(playerId) - e := checkExpired(l, l.Back) - if e == nil { - return nil, nil - } - return e.Value.(*NowPlayingInfo), nil -} - -func (r *nowPlayingRepository) count(playerId int) (int64, error) { - l := r.getList(playerId) - return int64(l.Len()), nil -} - -func checkExpired(l *list.List, f func() *list.Element) *list.Element { - for { - e := f() - if e == nil { - return nil - } - start := e.Value.(*NowPlayingInfo).Start - if time.Since(start) < NowPlayingExpire { - return e - } - l.Remove(e) - } -} diff --git a/core/nowplaying_test.go b/core/nowplaying_test.go deleted file mode 100644 index f33e910f..00000000 --- a/core/nowplaying_test.go +++ /dev/null @@ -1,61 +0,0 @@ -package core - -import ( - "sync" - "time" - - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" -) - -var _ = Describe("NowPlaying", func() { - var repo *nowPlayingRepository - var now = time.Now() - var past = time.Time{} - - BeforeEach(func() { - playerMap = sync.Map{} - repo = NewNowPlayingRepository().(*nowPlayingRepository) - }) - - It("enqueues and dequeues records", func() { - Expect(repo.Enqueue(&NowPlayingInfo{PlayerId: 1, TrackID: "AAA", Start: now})).To(BeNil()) - Expect(repo.Enqueue(&NowPlayingInfo{PlayerId: 1, TrackID: "BBB", Start: now})).To(BeNil()) - - Expect(repo.tail(1)).To(Equal(&NowPlayingInfo{PlayerId: 1, TrackID: "AAA", Start: now})) - Expect(repo.head(1)).To(Equal(&NowPlayingInfo{PlayerId: 1, TrackID: "BBB", Start: now})) - - Expect(repo.count(1)).To(Equal(int64(2))) - - Expect(repo.dequeue(1)).To(Equal(&NowPlayingInfo{PlayerId: 1, TrackID: "AAA", Start: now})) - Expect(repo.count(1)).To(Equal(int64(1))) - }) - - It("handles multiple players", func() { - Expect(repo.Enqueue(&NowPlayingInfo{PlayerId: 1, TrackID: "AAA", Start: now})).To(BeNil()) - Expect(repo.Enqueue(&NowPlayingInfo{PlayerId: 1, TrackID: "BBB", Start: now})).To(BeNil()) - - Expect(repo.Enqueue(&NowPlayingInfo{PlayerId: 2, TrackID: "CCC", Start: now})).To(BeNil()) - Expect(repo.Enqueue(&NowPlayingInfo{PlayerId: 2, TrackID: "DDD", Start: now})).To(BeNil()) - - Expect(repo.GetAll()).To(ConsistOf([]*NowPlayingInfo{ - {PlayerId: 1, TrackID: "BBB", Start: now}, - {PlayerId: 2, TrackID: "DDD", Start: now}, - })) - - Expect(repo.count(2)).To(Equal(int64(2))) - Expect(repo.count(2)).To(Equal(int64(2))) - - Expect(repo.tail(1)).To(Equal(&NowPlayingInfo{PlayerId: 1, TrackID: "AAA", Start: now})) - Expect(repo.head(2)).To(Equal(&NowPlayingInfo{PlayerId: 2, TrackID: "DDD", Start: now})) - }) - - It("handles expired items", func() { - Expect(repo.Enqueue(&NowPlayingInfo{PlayerId: 1, TrackID: "AAA", Start: past})).To(BeNil()) - Expect(repo.Enqueue(&NowPlayingInfo{PlayerId: 2, TrackID: "BBB", Start: now})).To(BeNil()) - - Expect(repo.GetAll()).To(ConsistOf([]*NowPlayingInfo{ - {PlayerId: 2, TrackID: "BBB", Start: now}, - })) - }) -}) diff --git a/core/scrobbler/scrobbler.go b/core/scrobbler/scrobbler.go new file mode 100644 index 00000000..d6ba4ea0 --- /dev/null +++ b/core/scrobbler/scrobbler.go @@ -0,0 +1,73 @@ +package scrobbler + +import ( + "context" + "sort" + "sync" + "time" + + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/utils/singleton" +) + +const nowPlayingExpire = 60 * time.Minute + +type NowPlayingInfo struct { + TrackID string + Start time.Time + Username string + PlayerId int + PlayerName string +} + +type Scrobbler interface { + NowPlaying(ctx context.Context, playerId int, playerName string, trackId string) error + GetNowPlaying(ctx context.Context) ([]NowPlayingInfo, error) + Submit(ctx context.Context, playerId int, trackId string, playTime time.Time) error +} + +type scrobbler struct { + ds model.DataStore +} + +var playMap = sync.Map{} + +func New(ds model.DataStore) Scrobbler { + instance := singleton.Get(scrobbler{}, func() interface{} { + return &scrobbler{ds: ds} + }) + return instance.(*scrobbler) +} + +func (s *scrobbler) NowPlaying(ctx context.Context, playerId int, playerName string, trackId string) error { + username, _ := request.UsernameFrom(ctx) + info := NowPlayingInfo{ + TrackID: trackId, + Start: time.Now(), + Username: username, + PlayerId: playerId, + PlayerName: playerName, + } + playMap.Store(playerId, info) + return nil +} + +func (s *scrobbler) GetNowPlaying(ctx context.Context) ([]NowPlayingInfo, error) { + var res []NowPlayingInfo + playMap.Range(func(playerId, value interface{}) bool { + info := value.(NowPlayingInfo) + if time.Since(info.Start) < nowPlayingExpire { + res = append(res, info) + } + return true + }) + sort.Slice(res, func(i, j int) bool { + return res[i].Start.After(res[j].Start) + }) + return res, nil +} + +func (s *scrobbler) Submit(ctx context.Context, playerId int, trackId string, playTime time.Time) error { + panic("implement me") +} diff --git a/core/wire_providers.go b/core/wire_providers.go index d308a3c0..1bda91f0 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -2,6 +2,7 @@ package core import ( "github.com/google/wire" + "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/core/transcoder" ) @@ -11,10 +12,10 @@ var Set = wire.NewSet( GetTranscodingCache, GetImageCache, NewArchiver, - NewNowPlayingRepository, NewExternalMetadata, NewCacheWarmer, NewPlayers, transcoder.New, + scrobbler.New, NewShare, ) diff --git a/git/pre-commit b/git/pre-commit index f907dbba..12bb32d4 100755 --- a/git/pre-commit +++ b/git/pre-commit @@ -12,7 +12,7 @@ gofmtcmd="go run golang.org/x/tools/cmd/goimports" -gofiles=$(git diff --cached --name-only --diff-filter=ACM | grep '.go$') +gofiles=$(git diff --cached --name-only --diff-filter=ACM | grep '.go$' | grep -v '_gen.go$') [ -z "$gofiles" ] && exit 0 unformatted=$($gofmtcmd -l $gofiles) diff --git a/server/subsonic/album_lists.go b/server/subsonic/album_lists.go index 8085dcb8..a6df68be 100644 --- a/server/subsonic/album_lists.go +++ b/server/subsonic/album_lists.go @@ -6,7 +6,7 @@ import ( "net/http" "time" - "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/server/subsonic/filter" @@ -15,14 +15,14 @@ import ( ) type AlbumListController struct { - ds model.DataStore - nowPlaying core.NowPlaying + ds model.DataStore + scrobbler scrobbler.Scrobbler } -func NewAlbumListController(ds model.DataStore, nowPlaying core.NowPlaying) *AlbumListController { +func NewAlbumListController(ds model.DataStore, scrobbler scrobbler.Scrobbler) *AlbumListController { c := &AlbumListController{ - ds: ds, - nowPlaying: nowPlaying, + ds: ds, + scrobbler: scrobbler, } return c } @@ -134,7 +134,7 @@ func (c *AlbumListController) GetStarred2(w http.ResponseWriter, r *http.Request func (c *AlbumListController) GetNowPlaying(w http.ResponseWriter, r *http.Request) (*responses.Subsonic, error) { ctx := r.Context() - npInfo, err := c.nowPlaying.GetAll() + npInfo, err := c.scrobbler.GetNowPlaying(ctx) if err != nil { log.Error(r, "Error retrieving now playing list", "error", err) return nil, err diff --git a/server/subsonic/api.go b/server/subsonic/api.go index b50b26a7..98c25077 100644 --- a/server/subsonic/api.go +++ b/server/subsonic/api.go @@ -11,6 +11,7 @@ import ( "github.com/go-chi/chi/v5/middleware" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/scanner" @@ -33,10 +34,11 @@ type Router struct { ExternalMetadata core.ExternalMetadata Scanner scanner.Scanner Broker events.Broker + Scrobbler scrobbler.Scrobbler } func New(ds model.DataStore, artwork core.Artwork, streamer core.MediaStreamer, archiver core.Archiver, players core.Players, - externalMetadata core.ExternalMetadata, scanner scanner.Scanner, broker events.Broker) *Router { + externalMetadata core.ExternalMetadata, scanner scanner.Scanner, broker events.Broker, scrobbler scrobbler.Scrobbler) *Router { r := &Router{ DataStore: ds, Artwork: artwork, @@ -46,6 +48,7 @@ func New(ds model.DataStore, artwork core.Artwork, streamer core.MediaStreamer, ExternalMetadata: externalMetadata, Scanner: scanner, Broker: broker, + Scrobbler: scrobbler, } r.Handler = r.routes() return r diff --git a/server/subsonic/media_annotation.go b/server/subsonic/media_annotation.go index 8fd8b7c8..cfb55000 100644 --- a/server/subsonic/media_annotation.go +++ b/server/subsonic/media_annotation.go @@ -7,6 +7,7 @@ import ( "time" "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" @@ -16,13 +17,13 @@ import ( ) type MediaAnnotationController struct { - ds model.DataStore - npRepo core.NowPlaying - broker events.Broker + ds model.DataStore + scrobbler scrobbler.Scrobbler + broker events.Broker } -func NewMediaAnnotationController(ds model.DataStore, npr core.NowPlaying, broker events.Broker) *MediaAnnotationController { - return &MediaAnnotationController{ds: ds, npRepo: npr, broker: broker} +func NewMediaAnnotationController(ds model.DataStore, scrobbler scrobbler.Scrobbler, broker events.Broker) *MediaAnnotationController { + return &MediaAnnotationController{ds: ds, scrobbler: scrobbler, broker: broker} } func (c *MediaAnnotationController) SetRating(w http.ResponseWriter, r *http.Request) (*responses.Subsonic, error) { @@ -148,7 +149,7 @@ func (c *MediaAnnotationController) Scrobble(w http.ResponseWriter, r *http.Requ submissions++ event.With("song", mf.ID).With("album", mf.AlbumID).With("artist", mf.AlbumArtistID) } else { - _, err := c.scrobblerNowPlaying(ctx, playerId, playerName, id, username) + err := c.scrobblerNowPlaying(ctx, playerId, playerName, id, username) if err != nil { log.Error(r, "Error setting current song", "id", id, err) continue @@ -191,20 +192,20 @@ func (c *MediaAnnotationController) scrobblerRegister(ctx context.Context, playe return mf, err } -func (c *MediaAnnotationController) scrobblerNowPlaying(ctx context.Context, playerId int, playerName, trackId, username string) (*model.MediaFile, error) { +func (c *MediaAnnotationController) scrobblerNowPlaying(ctx context.Context, playerId int, playerName, trackId, username string) error { mf, err := c.ds.MediaFile(ctx).Get(trackId) if err != nil { - return nil, err + return err } if mf == nil { - return nil, fmt.Errorf(`ID "%s" not found`, trackId) + return fmt.Errorf(`ID "%s" not found`, trackId) } log.Info("Now Playing", "title", mf.Title, "artist", mf.Artist, "user", username) - info := &core.NowPlayingInfo{TrackID: trackId, Username: username, Start: time.Now(), PlayerId: playerId, PlayerName: playerName} - return mf, c.npRepo.Enqueue(info) + err = c.scrobbler.NowPlaying(ctx, playerId, playerName, trackId) + return err } func (c *MediaAnnotationController) setStar(ctx context.Context, star bool, ids ...string) error { diff --git a/server/subsonic/responses/responses.go b/server/subsonic/responses/responses.go index 57276139..c88c0284 100644 --- a/server/subsonic/responses/responses.go +++ b/server/subsonic/responses/responses.go @@ -243,9 +243,9 @@ type Starred struct { type NowPlayingEntry struct { Child - UserName string `xml:"username,attr" json:"username,omitempty"` - MinutesAgo int `xml:"minutesAgo,attr" json:"minutesAgo,omitempty"` - PlayerId int `xml:"playerId,attr" json:"playerId,omitempty"` + UserName string `xml:"username,attr" json:"username"` + MinutesAgo int `xml:"minutesAgo,attr" json:"minutesAgo"` + PlayerId int `xml:"playerId,attr" json:"playerId"` PlayerName string `xml:"playerName,attr" json:"playerName,omitempty"` } diff --git a/server/subsonic/wire_gen.go b/server/subsonic/wire_gen.go index bc9f46c8..a3d8b918 100644 --- a/server/subsonic/wire_gen.go +++ b/server/subsonic/wire_gen.go @@ -7,7 +7,6 @@ package subsonic import ( "github.com/google/wire" - "github.com/navidrome/navidrome/core" ) // Injectors from wire_injectors.go: @@ -26,16 +25,16 @@ func initBrowsingController(router *Router) *BrowsingController { func initAlbumListController(router *Router) *AlbumListController { dataStore := router.DataStore - nowPlaying := core.NewNowPlayingRepository() - albumListController := NewAlbumListController(dataStore, nowPlaying) + scrobbler := router.Scrobbler + albumListController := NewAlbumListController(dataStore, scrobbler) return albumListController } func initMediaAnnotationController(router *Router) *MediaAnnotationController { dataStore := router.DataStore - nowPlaying := core.NewNowPlayingRepository() + scrobbler := router.Scrobbler broker := router.Broker - mediaAnnotationController := NewMediaAnnotationController(dataStore, nowPlaying, broker) + mediaAnnotationController := NewMediaAnnotationController(dataStore, scrobbler, broker) return mediaAnnotationController } @@ -96,5 +95,15 @@ var allProviders = wire.NewSet( NewMediaRetrievalController, NewStreamController, NewBookmarksController, - NewLibraryScanningController, core.NewNowPlayingRepository, wire.FieldsOf(new(*Router), "DataStore", "Artwork", "Streamer", "Archiver", "ExternalMetadata", "Scanner", "Broker"), + NewLibraryScanningController, wire.FieldsOf( + new(*Router), + "DataStore", + "Artwork", + "Streamer", + "Archiver", + "ExternalMetadata", + "Scanner", + "Broker", + "Scrobbler", + ), ) diff --git a/server/subsonic/wire_injectors.go b/server/subsonic/wire_injectors.go index 3a736e96..75a6c82d 100644 --- a/server/subsonic/wire_injectors.go +++ b/server/subsonic/wire_injectors.go @@ -4,7 +4,6 @@ package subsonic import ( "github.com/google/wire" - "github.com/navidrome/navidrome/core" ) var allProviders = wire.NewSet( @@ -19,8 +18,17 @@ var allProviders = wire.NewSet( NewStreamController, NewBookmarksController, NewLibraryScanningController, - core.NewNowPlayingRepository, - wire.FieldsOf(new(*Router), "DataStore", "Artwork", "Streamer", "Archiver", "ExternalMetadata", "Scanner", "Broker"), + wire.FieldsOf( + new(*Router), + "DataStore", + "Artwork", + "Streamer", + "Archiver", + "ExternalMetadata", + "Scanner", + "Broker", + "Scrobbler", + ), ) func initSystemController(router *Router) *SystemController { diff --git a/utils/singleton/singleton.go b/utils/singleton/singleton.go new file mode 100644 index 00000000..fb1d86d4 --- /dev/null +++ b/utils/singleton/singleton.go @@ -0,0 +1,48 @@ +package singleton + +import ( + "reflect" + "strings" + + "github.com/navidrome/navidrome/log" +) + +var ( + instances = make(map[string]interface{}) + getOrCreateC = make(chan *entry, 1) +) + +type entry struct { + constructor func() interface{} + object interface{} + resultC chan interface{} +} + +// Get returns an existing instance of object. If it is not yet created, calls `constructor`, stores the +// result for future calls and return it +func Get(object interface{}, constructor func() interface{}) interface{} { + e := &entry{ + constructor: constructor, + object: object, + resultC: make(chan interface{}), + } + getOrCreateC <- e + return <-e.resultC +} + +func init() { + go func() { + for { + e := <-getOrCreateC + name := reflect.TypeOf(e.object).String() + name = strings.TrimPrefix(name, "*") + v, created := instances[name] + if !created { + v = e.constructor() + log.Trace("Created new singleton", "object", name, "instance", v) + instances[name] = v + } + e.resultC <- v + } + }() +} diff --git a/utils/singleton/singleton_test.go b/utils/singleton/singleton_test.go new file mode 100644 index 00000000..716b725a --- /dev/null +++ b/utils/singleton/singleton_test.go @@ -0,0 +1,53 @@ +package singleton_test + +import ( + "testing" + + "github.com/navidrome/navidrome/utils/singleton" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +func TestSingleton(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Singleton Suite") +} + +var _ = Describe("Get", func() { + type T struct{ val int } + var wasCalled bool + var instance interface{} + constructor := func() interface{} { + wasCalled = true + return &T{} + } + + BeforeEach(func() { + instance = singleton.Get(T{}, constructor) + }) + + It("calls the constructor to create a new instance", func() { + Expect(wasCalled).To(BeTrue()) + Expect(instance).To(BeAssignableToTypeOf(&T{})) + }) + + It("does not call the constructor the next time", func() { + instance.(*T).val = 10 + wasCalled = false + + newInstance := singleton.Get(T{}, constructor) + + Expect(newInstance.(*T).val).To(Equal(10)) + Expect(wasCalled).To(BeFalse()) + }) + + It("does not call the constructor even if a pointer is passed as the object", func() { + instance.(*T).val = 20 + wasCalled = false + + newInstance := singleton.Get(&T{}, constructor) + + Expect(newInstance.(*T).val).To(Equal(20)) + Expect(wasCalled).To(BeFalse()) + }) +})