diff --git a/CHANGELOG.md b/CHANGELOG.md index 8794b14..c977793 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/api/mojang/queue/queue.go b/api/mojang/queue/queue.go index 6a1d540..ff9900d 100644 --- a/api/mojang/queue/queue.go +++ b/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) diff --git a/api/mojang/queue/queue_test.go b/api/mojang/queue/queue_test.go index 120e831..8864df2 100644 --- a/api/mojang/queue/queue_test.go +++ b/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) { - m.Called(username, uuid) +func (m *mockStorage) StoreUuid(username string, uuid string) error { + 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", "Thinkofdeath", "4566e69fc90748ee8d71d7ba5aa00d20").Once() + suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil) + 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)) diff --git a/api/mojang/queue/storage.go b/api/mojang/queue/storage.go index 5b06ea1..2629e58 100644 --- a/api/mojang/queue/storage.go +++ b/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) { - s.UuidsStorage.StoreUuid(username, uuid) +func (s *SplittedStorage) StoreUuid(username string, uuid string) error { + return s.UuidsStorage.StoreUuid(username, uuid) } func (s *SplittedStorage) GetTextures(uuid string) (*mojang.SignedTexturesResponse, error) { diff --git a/api/mojang/queue/storage_test.go b/api/mojang/queue/storage_test.go index 66e3bd2..b9ee64c 100644 --- a/api/mojang/queue/storage_test.go +++ b/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) }) diff --git a/db/redis.go b/db/redis.go index 977c134..df07135 100644 --- a/db/redis.go +++ b/db/redis.go @@ -6,7 +6,6 @@ import ( "encoding/json" "fmt" "io" - "log" "strconv" "strings" "time" @@ -21,36 +20,35 @@ import ( ) type RedisFactory struct { - Host string - Port int - PoolSize int - connection *pool.Pool + Host string + Port int + PoolSize int + 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) { - connection, err := f.getConnection() +func (f *RedisFactory) createInstance() (*redisDb, error) { + 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) { - if f.connection == nil { +func (f *RedisFactory) getPool() (*pool.Pool, error) { + 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 - - 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") - 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") - } - }() + f.pool = conn } - return f.connection, nil + return f.pool, 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() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + 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() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + 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() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + 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() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + 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() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + 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() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + if err != nil { + return "", err + } + defer db.pool.Put(conn) return findMojangUuidByUsername(username, conn) } -func (db *redisDb) StoreUuid(username string, uuid string) { - conn, _ := db.conn.Get() - defer db.conn.Put(conn) +func (db *redisDb) StoreUuid(username string, uuid string) error { + conn, err := db.pool.Get() + 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 { - return err + if _, ok := err.(*SkinNotFoundError); ok { + return nil } + + return err } conn.Cmd("MULTI") conn.Cmd("DEL", buildUsernameKey(record.Username)) - if record != nil { - conn.Cmd("HDEL", accountIdToUsernameKey, record.UserId) - } + 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.Close() + _, _ = writer.Write(str) + _ = 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