diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/providers/RedisClientFactory.java b/service/src/main/java/org/whispersystems/textsecuregcm/providers/RedisClientFactory.java index 7991fa621..f2e138451 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/providers/RedisClientFactory.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/providers/RedisClientFactory.java @@ -48,6 +48,7 @@ public class RedisClientFactory implements RedisPubSubConnectionFactory { { JedisPoolConfig poolConfig = new JedisPoolConfig(); poolConfig.setTestOnBorrow(true); + poolConfig.setMaxWaitMillis(10000); URI redisURI = new URI(url); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/redis/LuaScript.java b/service/src/main/java/org/whispersystems/textsecuregcm/redis/LuaScript.java index 24d067d67..c03d64160 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/redis/LuaScript.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/redis/LuaScript.java @@ -39,19 +39,27 @@ public class LuaScript { public Object execute(List keys, List args) { try (Jedis jedis = jedisPool.getWriteResource()) { - try { - return jedis.evalsha(sha, keys, args); - } catch (JedisDataException e) { - storeScript(jedisPool, script); - return jedis.evalsha(sha, keys, args); - } + return execute(jedis, keys, args); + } + } + + public Object execute(Jedis jedis, List keys, List args) { + try { + return jedis.evalsha(sha, keys, args); + } catch (JedisDataException e) { + storeScript(jedis, script); + return jedis.evalsha(sha, keys, args); } } private String storeScript(ReplicatedJedisPool jedisPool, String script) { try (Jedis jedis = jedisPool.getWriteResource()) { - return jedis.scriptLoad(script); + return storeScript(jedis, script); } } + private String storeScript(Jedis jedis, String script) { + return jedis.scriptLoad(script); + } + } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index 24044c295..874138476 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -67,7 +67,7 @@ public class AccountsManager { } public boolean create(Account account) { - try (Timer.Context context = createTimer.time()) { + try (Timer.Context ignored = createTimer.time()) { boolean freshUser = databaseCreate(account); redisSet(account); updateDirectory(account); @@ -77,7 +77,7 @@ public class AccountsManager { } public void update(Account account) { - try (Timer.Context context = updateTimer.time()) { + try (Timer.Context ignored = updateTimer.time()) { redisSet(account); databaseUpdate(account); updateDirectory(account); @@ -91,7 +91,7 @@ public class AccountsManager { } public Optional get(String number) { - try (Timer.Context context = getByNumberTimer.time()) { + try (Timer.Context ignored = getByNumberTimer.time()) { Optional account = redisGet(number); if (!account.isPresent()) { @@ -104,7 +104,7 @@ public class AccountsManager { } public Optional get(UUID uuid) { - try (Timer.Context context = getByUuidTimer.time()) { + try (Timer.Context ignored = getByUuidTimer.time()) { Optional account = redisGet(uuid); if (!account.isPresent()) { @@ -140,12 +140,12 @@ public class AccountsManager { } private String getAccountEntityKey(UUID uuid) { - return "Account::" + uuid.toString(); + return "Account2::" + uuid.toString(); } private void redisSet(Account account) { - try (Jedis jedis = cacheClient.getWriteResource(); - Timer.Context timer = redisSetTimer.time()) + try (Jedis jedis = cacheClient.getWriteResource(); + Timer.Context ignored = redisSetTimer.time()) { jedis.set(getAccountMapKey(account.getNumber()), account.getUuid().toString()); jedis.set(getAccountEntityKey(account.getUuid()), mapper.writeValueAsString(account)); @@ -155,12 +155,12 @@ public class AccountsManager { } private Optional redisGet(String number) { - try (Jedis jedis = cacheClient.getReadResource(); - Timer.Context timer = redisNumberGetTimer.time()) + try (Jedis jedis = cacheClient.getReadResource(); + Timer.Context ignored = redisNumberGetTimer.time()) { String uuid = jedis.get(getAccountMapKey(number)); - if (uuid != null) return redisGet(UUID.fromString(uuid)); + if (uuid != null) return redisGet(jedis, UUID.fromString(uuid)); else return Optional.empty(); } catch (IllegalArgumentException e) { logger.warn("Deserialization error", e); @@ -172,9 +172,13 @@ public class AccountsManager { } private Optional redisGet(UUID uuid) { - try (Jedis jedis = cacheClient.getReadResource(); - Timer.Context timer = redisUuidGetTimer.time()) - { + try (Jedis jedis = cacheClient.getReadResource()) { + return redisGet(jedis, uuid); + } + } + + private Optional redisGet(Jedis jedis, UUID uuid) { + try (Timer.Context ignored = redisUuidGetTimer.time()) { String json = jedis.get(getAccountEntityKey(uuid)); if (json != null) { @@ -192,7 +196,6 @@ public class AccountsManager { logger.warn("Redis failure", e); return Optional.empty(); } - } private Optional databaseGet(String number) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagesCache.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagesCache.java index 0a3d2a8df..44d3f02a1 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagesCache.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/MessagesCache.java @@ -82,12 +82,10 @@ public class MessagesCache implements Managed { } public void remove(String destination, long destinationDevice, long id) { - Timer.Context timer = removeByIdTimer.time(); - - try { - removeOperation.remove(destination, destinationDevice, id); - } finally { - timer.stop(); + try (Jedis jedis = jedisPool.getWriteResource(); + Timer.Context ignored = removeByIdTimer.time()) + { + removeOperation.remove(jedis, destination, destinationDevice, id); } } @@ -298,13 +296,13 @@ public class MessagesCache implements Managed { this.removeQueue = LuaScript.fromResource(jedisPool, "lua/remove_queue.lua" ); } - public void remove(String destination, long destinationDevice, long id) { + public void remove(Jedis jedis, String destination, long destinationDevice, long id) { Key key = new Key(destination, destinationDevice); List keys = Arrays.asList(key.getUserMessageQueue(), key.getUserMessageQueueMetadata(), Key.getUserMessageQueueIndex()); List args = Collections.singletonList(String.valueOf(id).getBytes()); - this.removeById.execute(keys, args); + this.removeById.execute(jedis, keys, args); } public byte[] remove(String destination, long destinationDevice, String sender, long timestamp) { @@ -464,7 +462,7 @@ public class MessagesCache implements Managed { Set messages = jedis.zrangeWithScores(key.getUserMessageQueue(), 0, CHUNK_SIZE); for (Tuple message : messages) { - persistMessage(key, (long)message.getScore(), message.getBinaryElement()); + persistMessage(jedis, key, (long)message.getScore(), message.getBinaryElement()); messagesPersistedCount++; } @@ -479,7 +477,7 @@ public class MessagesCache implements Managed { } } - private void persistMessage(Key key, long score, byte[] message) { + private void persistMessage(Jedis jedis, Key key, long score, byte[] message) { try { Envelope envelope = Envelope.parseFrom(message); UUID guid = envelope.hasServerGuid() ? UUID.fromString(envelope.getServerGuid()) : null; @@ -491,7 +489,7 @@ public class MessagesCache implements Managed { logger.error("Error parsing envelope", e); } - removeOperation.remove(key.getAddress(), key.getDeviceId(), score); + removeOperation.remove(jedis, key.getAddress(), key.getDeviceId(), score); } private List getQueuesToPersist(GetOperation getOperation) { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java index e49389821..2baed4259 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountsManagerTest.java @@ -32,7 +32,7 @@ public class AccountsManagerTest { when(cacheClient.getReadResource()).thenReturn(jedis); when(jedis.get(eq("AccountMap::+14152222222"))).thenReturn(uuid.toString()); - when(jedis.get(eq("Account::" + uuid.toString()))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\"}"); + when(jedis.get(eq("Account2::" + uuid.toString()))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\"}"); AccountsManager accountsManager = new AccountsManager(accounts, directoryManager, cacheClient); Optional account = accountsManager.get("+14152222222"); @@ -42,8 +42,8 @@ public class AccountsManagerTest { assertEquals(account.get().getProfileName(), "test"); verify(jedis, times(1)).get(eq("AccountMap::+14152222222")); - verify(jedis, times(1)).get(eq("Account::" + uuid.toString())); - verify(jedis, times(2)).close(); + verify(jedis, times(1)).get(eq("Account2::" + uuid.toString())); + verify(jedis, times(1)).close(); verifyNoMoreInteractions(jedis); verifyNoMoreInteractions(accounts); } @@ -58,7 +58,7 @@ public class AccountsManagerTest { UUID uuid = UUID.randomUUID(); when(cacheClient.getReadResource()).thenReturn(jedis); - when(jedis.get(eq("Account::" + uuid.toString()))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\"}"); + when(jedis.get(eq("Account2::" + uuid.toString()))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\"}"); AccountsManager accountsManager = new AccountsManager(accounts, directoryManager, cacheClient); Optional account = accountsManager.get(uuid); @@ -68,7 +68,7 @@ public class AccountsManagerTest { assertEquals(account.get().getUuid(), uuid); assertEquals(account.get().getProfileName(), "test"); - verify(jedis, times(1)).get(eq("Account::" + uuid.toString())); + verify(jedis, times(1)).get(eq("Account2::" + uuid.toString())); verify(jedis, times(1)).close(); verifyNoMoreInteractions(jedis); verifyNoMoreInteractions(accounts); @@ -97,7 +97,7 @@ public class AccountsManagerTest { verify(jedis, times(1)).get(eq("AccountMap::+14152222222")); verify(jedis, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(jedis, times(1)).set(eq("Account::" + uuid.toString()), anyString()); + verify(jedis, times(1)).set(eq("Account2::" + uuid.toString()), anyString()); verify(jedis, times(2)).close(); verifyNoMoreInteractions(jedis); @@ -116,7 +116,7 @@ public class AccountsManagerTest { when(cacheClient.getReadResource()).thenReturn(jedis); when(cacheClient.getWriteResource()).thenReturn(jedis); - when(jedis.get(eq("Account::" + uuid))).thenReturn(null); + when(jedis.get(eq("Account2::" + uuid))).thenReturn(null); when(accounts.get(eq(uuid))).thenReturn(Optional.of(account)); AccountsManager accountsManager = new AccountsManager(accounts, directoryManager, cacheClient); @@ -125,9 +125,9 @@ public class AccountsManagerTest { assertTrue(retrieved.isPresent()); assertSame(retrieved.get(), account); - verify(jedis, times(1)).get(eq("Account::" + uuid)); + verify(jedis, times(1)).get(eq("Account2::" + uuid)); verify(jedis, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(jedis, times(1)).set(eq("Account::" + uuid.toString()), anyString()); + verify(jedis, times(1)).set(eq("Account2::" + uuid.toString()), anyString()); verify(jedis, times(2)).close(); verifyNoMoreInteractions(jedis); @@ -157,7 +157,7 @@ public class AccountsManagerTest { verify(jedis, times(1)).get(eq("AccountMap::+14152222222")); verify(jedis, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(jedis, times(1)).set(eq("Account::" + uuid.toString()), anyString()); + verify(jedis, times(1)).set(eq("Account2::" + uuid.toString()), anyString()); verify(jedis, times(2)).close(); verifyNoMoreInteractions(jedis); @@ -176,7 +176,7 @@ public class AccountsManagerTest { when(cacheClient.getReadResource()).thenReturn(jedis); when(cacheClient.getWriteResource()).thenReturn(jedis); - when(jedis.get(eq("Account::" + uuid))).thenThrow(new JedisException("Connection lost!")); + when(jedis.get(eq("Account2::" + uuid))).thenThrow(new JedisException("Connection lost!")); when(accounts.get(eq(uuid))).thenReturn(Optional.of(account)); AccountsManager accountsManager = new AccountsManager(accounts, directoryManager, cacheClient); @@ -185,9 +185,9 @@ public class AccountsManagerTest { assertTrue(retrieved.isPresent()); assertSame(retrieved.get(), account); - verify(jedis, times(1)).get(eq("Account::" + uuid)); + verify(jedis, times(1)).get(eq("Account2::" + uuid)); verify(jedis, times(1)).set(eq("AccountMap::+14152222222"), eq(uuid.toString())); - verify(jedis, times(1)).set(eq("Account::" + uuid.toString()), anyString()); + verify(jedis, times(1)).set(eq("Account2::" + uuid.toString()), anyString()); verify(jedis, times(2)).close(); verifyNoMoreInteractions(jedis);