Adjust Mojang's queue behavior

CHANGELOG.md

@@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Mojang's textures queue now caches textures data for 70 seconds to avoid strange `429` errors.
 - Mojang's textures queue now doesn't log timeout errors.
 
+### Fixed
+- Panic when Redis connection is broken.
+- Duplication of Redis connections pool for Mojang's textures queue.
+
 ## [4.2.0] - 2019-05-02
 ### Added
 - `CHANGELOG.md` file.

api/mojang/queue/queue.go

@@ -143,7 +143,7 @@ func (ctx *JobsQueue) queueRound() {
 				}
 			}
 
-			ctx.Storage.StoreUuid(job.Username, uuid)
+			_ = ctx.Storage.StoreUuid(job.Username, uuid)
 			if uuid == "" {
 				job.RespondTo <- nil
 				ctx.Logger.IncCounter("mojang_textures.usernames.uuid_miss", 1)

api/mojang/queue/queue_test.go

@@ -53,8 +53,9 @@ func (m *mockStorage) GetUuid(username string) (string, error) {
 	return args.String(0), args.Error(1)
 }
 
-func (m *mockStorage) StoreUuid(username string, uuid string) {
+func (m *mockStorage) StoreUuid(username string, uuid string) error {
-	m.Called(username, uuid)
+	args := m.Called(username, uuid)
+	return args.Error(0)
 }
 
 func (m *mockStorage) GetTextures(uuid string) (*mojang.SignedTexturesResponse, error) {
@@ -133,7 +134,7 @@ func (suite *queueTestSuite) TestReceiveTexturesForOneUsernameWithoutAnyCache()
 	suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Once()
 
 	suite.Storage.On("GetUuid", "maksimkurb").Once().Return("", &ValueNotFound{})
-	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once()
+	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil)
 	suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil, &ValueNotFound{})
 	suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", expectedResult).Once()
 
@@ -166,8 +167,8 @@ func (suite *queueTestSuite) TestReceiveTexturesForFewUsernamesWithoutAnyCache()
 
 	suite.Storage.On("GetUuid", "maksimkurb").Once().Return("", &ValueNotFound{})
 	suite.Storage.On("GetUuid", "Thinkofdeath").Once().Return("", &ValueNotFound{})
-	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once()
+	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil)
-	suite.Storage.On("StoreUuid", "Thinkofdeath", "4566e69fc90748ee8d71d7ba5aa00d20").Once()
+	suite.Storage.On("StoreUuid", "Thinkofdeath", "4566e69fc90748ee8d71d7ba5aa00d20").Once().Return(nil)
 	suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil, &ValueNotFound{})
 	suite.Storage.On("GetTextures", "4566e69fc90748ee8d71d7ba5aa00d20").Once().Return(nil, &ValueNotFound{})
 	suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", expectedResult1).Once()
@@ -274,7 +275,7 @@ func (suite *queueTestSuite) TestReceiveTexturesForMoreThan100Usernames() {
 	suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Times(120)
 
 	suite.Storage.On("GetUuid", mock.Anything).Times(120).Return("", &ValueNotFound{})
-	suite.Storage.On("StoreUuid", mock.Anything, "").Times(120) // if username is not compared to uuid, then receive ""
+	suite.Storage.On("StoreUuid", mock.Anything, "").Times(120).Return(nil) // should be called with "" if username is not compared to uuid
 	// Storage.GetTextures and Storage.SetTextures shouldn't be called
 
 	suite.MojangApi.On("UsernamesToUuids", usernames[0:100]).Once().Return([]*mojang.ProfileInfo{}, nil)
@@ -308,7 +309,7 @@ func (suite *queueTestSuite) TestReceiveTexturesForTheSameUsernames() {
 	suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Once()
 
 	suite.Storage.On("GetUuid", "maksimkurb").Twice().Return("", &ValueNotFound{})
-	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once()
+	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil)
 	suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil, &ValueNotFound{})
 	suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", expectedResult).Once()
 	suite.MojangApi.On("UsernamesToUuids", []string{"maksimkurb"}).Once().Return([]*mojang.ProfileInfo{
@@ -340,7 +341,7 @@ func (suite *queueTestSuite) TestReceiveTexturesForUsernameThatAlreadyProcessing
 	suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Once()
 
 	suite.Storage.On("GetUuid", "maksimkurb").Twice().Return("", &ValueNotFound{})
-	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once()
+	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil)
 	suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil, &ValueNotFound{})
 	suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", expectedResult).Once()
 
@@ -376,7 +377,7 @@ func (suite *queueTestSuite) TestDoNothingWhenNoTasks() {
 	suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Once()
 
 	suite.Storage.On("GetUuid", "maksimkurb").Once().Return("", &ValueNotFound{})
-	suite.Storage.On("StoreUuid", "maksimkurb", "").Once()
+	suite.Storage.On("StoreUuid", "maksimkurb", "").Once().Return(nil)
 	// Storage.GetTextures and Storage.StoreTextures shouldn't be called
 
 	suite.MojangApi.On("UsernamesToUuids", []string{"maksimkurb"}).Once().Return([]*mojang.ProfileInfo{}, nil)
@@ -455,7 +456,7 @@ func (suite *queueTestSuite) TestShouldNotLogErrorWhenExpectedErrorReturnedFromU
 	suite.Logger.On("Warning", ":name: Got 429 Too Many Requests :err", mock.Anything, mock.Anything).Once()
 
 	suite.Storage.On("GetUuid", "maksimkurb").Return("", &ValueNotFound{})
-	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32")
+	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Return(nil)
 	suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Return(nil, &ValueNotFound{})
 	suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", (*mojang.SignedTexturesResponse)(nil))
 
@@ -481,7 +482,7 @@ func (suite *queueTestSuite) TestShouldLogEmergencyOnUnexpectedErrorReturnedFrom
 	suite.Logger.On("Emergency", ":name: Unknown Mojang response error: :err", mock.Anything, mock.Anything).Once()
 
 	suite.Storage.On("GetUuid", "maksimkurb").Return("", &ValueNotFound{})
-	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32")
+	suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Return(nil)
 	suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Return(nil, &ValueNotFound{})
 	suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", (*mojang.SignedTexturesResponse)(nil))
 

api/mojang/queue/storage.go

@@ -4,7 +4,7 @@ import "github.com/elyby/chrly/api/mojang"
 
 type UuidsStorage interface {
 	GetUuid(username string) (string, error)
-	StoreUuid(username string, uuid string)
+	StoreUuid(username string, uuid string) error
 }
 
 // nil value can be passed to the storage to indicate that there is no textures
@@ -31,8 +31,8 @@ func (s *SplittedStorage) GetUuid(username string) (string, error) {
 	return s.UuidsStorage.GetUuid(username)
 }
 
-func (s *SplittedStorage) StoreUuid(username string, uuid string) {
+func (s *SplittedStorage) StoreUuid(username string, uuid string) error {
-	s.UuidsStorage.StoreUuid(username, uuid)
+	return s.UuidsStorage.StoreUuid(username, uuid)
 }
 
 func (s *SplittedStorage) GetTextures(uuid string) (*mojang.SignedTexturesResponse, error) {

api/mojang/queue/storage_test.go

@@ -17,8 +17,9 @@ func (m *uuidsStorageMock) GetUuid(username string) (string, error) {
 	return args.String(0), args.Error(1)
 }
 
-func (m *uuidsStorageMock) StoreUuid(username string, uuid string) {
+func (m *uuidsStorageMock) StoreUuid(username string, uuid string) error {
 	m.Called(username, uuid)
+	return nil
 }
 
 type texturesStorageMock struct {
@@ -59,7 +60,7 @@ func TestSplittedStorage(t *testing.T) {
 	t.Run("StoreUuid", func(t *testing.T) {
 		storage, uuidsMock, _ := createMockedStorage()
 		uuidsMock.On("StoreUuid", "username", "result").Once()
-		storage.StoreUuid("username", "result")
+		_ = storage.StoreUuid("username", "result")
 		uuidsMock.AssertExpectations(t)
 	})
 

db/redis.go

@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"log"
 	"strconv"
 	"strings"
 	"time"
@@ -24,33 +23,32 @@ type RedisFactory struct {
 	Host     string
 	Port     int
 	PoolSize int
-	connection *pool.Pool
+	pool     *pool.Pool
 }
 
-// TODO: Why isn't a pointer used here?
+func (f *RedisFactory) CreateSkinsRepository() (interfaces.SkinsRepository, error) {
-func (f RedisFactory) CreateSkinsRepository() (interfaces.SkinsRepository, error) {
 	return f.createInstance()
 }
 
-func (f RedisFactory) CreateCapesRepository() (interfaces.CapesRepository, error) {
+func (f *RedisFactory) CreateCapesRepository() (interfaces.CapesRepository, error) {
 	panic("capes repository not supported for this storage type")
 }
 
-func (f RedisFactory) CreateMojangUuidsRepository() (queue.UuidsStorage, error) {
+func (f *RedisFactory) CreateMojangUuidsRepository() (queue.UuidsStorage, error) {
 	return f.createInstance()
 }
 
-func (f RedisFactory) createInstance() (*redisDb, error) {
+func (f *RedisFactory) createInstance() (*redisDb, error) {
-	connection, err := f.getConnection()
+	p, err := f.getPool()
 	if err != nil {
 		return nil, err
 	}
 
-	return &redisDb{connection}, nil
+	return &redisDb{p}, nil
 }
 
-func (f RedisFactory) getConnection() (*pool.Pool, error) {
+func (f *RedisFactory) getPool() (*pool.Pool, error) {
-	if f.connection == nil {
+	if f.pool == nil {
 		if f.Host == "" {
 			return nil, &ParamRequired{"host"}
 		}
@@ -65,88 +63,87 @@ func (f RedisFactory) getConnection() (*pool.Pool, error) {
 			return nil, err
 		}
 
-		f.connection = conn
+		f.pool = conn
-
-		go func() {
-			period := 5
-			for {
-				time.Sleep(time.Duration(period) * time.Second)
-				resp := f.connection.Cmd("PING")
-				if resp.Err == nil {
-					continue
 	}
 
-				log.Println("Redis not pinged. Try to reconnect")
+	return f.pool, nil
-				conn, err := pool.New("tcp", addr, f.PoolSize)
-				if err != nil {
-					log.Printf("Cannot reconnect to redis: %v\n", err)
-					log.Printf("Waiting %d seconds to retry\n", period)
-					continue
-				}
-
-				f.connection = conn
-				log.Println("Reconnected")
-			}
-		}()
-	}
-
-	return f.connection, nil
 }
 
 type redisDb struct {
-	conn *pool.Pool
+	pool *pool.Pool
 }
 
 const accountIdToUsernameKey = "hash:username-to-account-id"
 const mojangUsernameToUuidKey = "hash:mojang-username-to-uuid"
 
 func (db *redisDb) FindByUsername(username string) (*model.Skin, error) {
-	conn, _ := db.conn.Get()
+	conn, err := db.pool.Get()
-	defer db.conn.Put(conn)
+	if err != nil {
+		return nil, err
+	}
+	defer db.pool.Put(conn)
 
 	return findByUsername(username, conn)
 }
 
 func (db *redisDb) FindByUserId(id int) (*model.Skin, error) {
-	conn, _ := db.conn.Get()
+	conn, err := db.pool.Get()
-	defer db.conn.Put(conn)
+	if err != nil {
+		return nil, err
+	}
+	defer db.pool.Put(conn)
 
 	return findByUserId(id, conn)
 }
 
 func (db *redisDb) Save(skin *model.Skin) error {
-	conn, _ := db.conn.Get()
+	conn, err := db.pool.Get()
-	defer db.conn.Put(conn)
+	if err != nil {
+		return err
+	}
+	defer db.pool.Put(conn)
 
 	return save(skin, conn)
 }
 
 func (db *redisDb) RemoveByUserId(id int) error {
-	conn, _ := db.conn.Get()
+	conn, err := db.pool.Get()
-	defer db.conn.Put(conn)
+	if err != nil {
+		return err
+	}
+	defer db.pool.Put(conn)
 
 	return removeByUserId(id, conn)
 }
 
 func (db *redisDb) RemoveByUsername(username string) error {
-	conn, _ := db.conn.Get()
+	conn, err := db.pool.Get()
-	defer db.conn.Put(conn)
+	if err != nil {
+		return err
+	}
+	defer db.pool.Put(conn)
 
 	return removeByUsername(username, conn)
 }
 
 func (db *redisDb) GetUuid(username string) (string, error) {
-	conn, _ := db.conn.Get()
+	conn, err := db.pool.Get()
-	defer db.conn.Put(conn)
+	if err != nil {
+		return "", err
+	}
+	defer db.pool.Put(conn)
 
 	return findMojangUuidByUsername(username, conn)
 }
 
-func (db *redisDb) StoreUuid(username string, uuid string) {
+func (db *redisDb) StoreUuid(username string, uuid string) error {
-	conn, _ := db.conn.Get()
+	conn, err := db.pool.Get()
-	defer db.conn.Put(conn)
+	if err != nil {
+		return err
+	}
+	defer db.pool.Put(conn)
 
-	storeMojangUuid(username, uuid, conn)
+	return storeMojangUuid(username, uuid, conn)
 }
 
 func findByUsername(username string, conn util.Cmder) (*model.Skin, error) {
@@ -156,7 +153,7 @@ func findByUsername(username string, conn util.Cmder) (*model.Skin, error) {
 
 	redisKey := buildUsernameKey(username)
 	response := conn.Cmd("GET", redisKey)
-	if response.IsType(redis.Nil) {
+	if !response.IsType(redis.Str) {
 		return nil, &SkinNotFoundError{username}
 	}
 
@@ -183,7 +180,7 @@ func findByUsername(username string, conn util.Cmder) (*model.Skin, error) {
 
 func findByUserId(id int, conn util.Cmder) (*model.Skin, error) {
 	response := conn.Cmd("HGET", accountIdToUsernameKey, id)
-	if response.IsType(redis.Nil) {
+	if !response.IsType(redis.Str) {
 		return nil, &SkinNotFoundError{"unknown"}
 	}
 
@@ -215,17 +212,17 @@ func removeByUserId(id int, conn util.Cmder) error {
 func removeByUsername(username string, conn util.Cmder) error {
 	record, err := findByUsername(username, conn)
 	if err != nil {
-		if _, ok := err.(*SkinNotFoundError); !ok {
+		if _, ok := err.(*SkinNotFoundError); ok {
-			return err
+			return nil
 		}
+
+		return err
 	}
 
 	conn.Cmd("MULTI")
 
 	conn.Cmd("DEL", buildUsernameKey(record.Username))
-	if record != nil {
 	conn.Cmd("HDEL", accountIdToUsernameKey, record.UserId)
-	}
 
 	conn.Cmd("EXEC")
 
@@ -272,9 +269,14 @@ func findMojangUuidByUsername(username string, conn util.Cmder) (string, error)
 	return parts[0], nil
 }
 
-func storeMojangUuid(username string, uuid string, conn util.Cmder) {
+func storeMojangUuid(username string, uuid string, conn util.Cmder) error {
 	value := uuid + ":" + strconv.FormatInt(time.Now().Unix(), 10)
-	conn.Cmd("HSET", mojangUsernameToUuidKey, strings.ToLower(username), value)
+	res := conn.Cmd("HSET", mojangUsernameToUuidKey, strings.ToLower(username), value)
+	if res.IsType(redis.Err) {
+		return res.Err
+	}
+
+	return nil
 }
 
 func buildUsernameKey(username string) string {
@@ -284,8 +286,8 @@ func buildUsernameKey(username string) string {
 func zlibEncode(str []byte) []byte {
 	var buff bytes.Buffer
 	writer := zlib.NewWriter(&buff)
-	writer.Write(str)
+	_, _ = writer.Write(str)
-	writer.Close()
+	_ = writer.Close()
 
 	return buff.Bytes()
 }
@@ -298,7 +300,7 @@ func zlibDecode(bts []byte) ([]byte, error) {
 	}
 
 	resultBuffer := new(bytes.Buffer)
-	io.Copy(resultBuffer, reader)
+	_, _ = io.Copy(resultBuffer, reader)
 	reader.Close()
 
 	return resultBuffer.Bytes(), nil