New implementation of NowPlaying

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
 }
 

core/nowplaying.go

@@ -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)
-	}
-}

core/nowplaying_test.go

@@ -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},
-		}))
-	})
-})

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")
+}

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,
 )

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)

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
+	ds        model.DataStore
-	nowPlaying core.NowPlaying
+	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,
+		ds:        ds,
-		nowPlaying: nowPlaying,
+		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

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

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
+	ds        model.DataStore
-	npRepo core.NowPlaying
+	scrobbler scrobbler.Scrobbler
-	broker events.Broker
+	broker    events.Broker
 }
 
-func NewMediaAnnotationController(ds model.DataStore, npr core.NowPlaying, broker events.Broker) *MediaAnnotationController {
+func NewMediaAnnotationController(ds model.DataStore, scrobbler scrobbler.Scrobbler, broker events.Broker) *MediaAnnotationController {
-	return &MediaAnnotationController{ds: ds, npRepo: npr, broker: broker}
+	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}
+	err = c.scrobbler.NowPlaying(ctx, playerId, playerName, trackId)
-	return mf, c.npRepo.Enqueue(info)
+	return err
 }
 
 func (c *MediaAnnotationController) setStar(ctx context.Context, star bool, ids ...string) error {

server/subsonic/responses/responses.go

@@ -243,9 +243,9 @@ type Starred struct {
 
 type NowPlayingEntry struct {
 	Child
-	UserName   string `xml:"username,attr"                        json:"username,omitempty"`
+	UserName   string `xml:"username,attr"                        json:"username"`
-	MinutesAgo int    `xml:"minutesAgo,attr"                      json:"minutesAgo,omitempty"`
+	MinutesAgo int    `xml:"minutesAgo,attr"                      json:"minutesAgo"`
-	PlayerId   int    `xml:"playerId,attr"                        json:"playerId,omitempty"`
+	PlayerId   int    `xml:"playerId,attr"                        json:"playerId"`
 	PlayerName string `xml:"playerName,attr"                      json:"playerName,omitempty"`
 }
 

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()
+	scrobbler := router.Scrobbler
-	albumListController := NewAlbumListController(dataStore, nowPlaying)
+	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",
+	),
 )

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(
-	wire.FieldsOf(new(*Router), "DataStore", "Artwork", "Streamer", "Archiver", "ExternalMetadata", "Scanner", "Broker"),
+		new(*Router),
+		"DataStore",
+		"Artwork",
+		"Streamer",
+		"Archiver",
+		"ExternalMetadata",
+		"Scanner",
+		"Broker",
+		"Scrobbler",
+	),
 )
 
 func initSystemController(router *Router) *SystemController {

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
+		}
+	}()
+}

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())
+	})
+})