#RI-3986 possible fix to deal with concurrent requests (reuse existing)

Author: Artem

redisinsight/api/src/__mocks__/redis.ts

@@ -68,7 +68,9 @@ export const mockRedisService = jest.fn(() => ({
   getClientInstance: jest.fn().mockResolvedValue({
     client: mockIORedisClient,
   }),
-  setClientInstance: jest.fn(),
+  setClientInstance: jest.fn().mockReturnValue({
+    client: mockIORedisClient,
+  }),
   isClientConnected: jest.fn().mockReturnValue(true),
   removeClientInstance: jest.fn(),
   removeClientInstances: jest.fn(),

redisinsight/api/src/modules/database/database-connection.service.ts

@@ -77,9 +77,7 @@ export class DatabaseConnectionService {
 
     client = await this.createClient(clientMetadata);
 
-    this.redisService.setClientInstance(clientMetadata, client);
-
-    return client;
+    return this.redisService.setClientInstance(clientMetadata, client)?.client;
   }
 
   /**

redisinsight/api/src/modules/redis/redis-consumer.abstract.service.spec.ts

@@ -54,6 +54,8 @@ describe('RedisConsumerAbstractService', () => {
     redisService = await module.get<RedisService>(RedisService);
     consumerInstance = await module.get<BrowserToolService>(BrowserToolService);
     redisConnectionFactory = await module.get(RedisConnectionFactory);
+
+    redisService.setClientInstance.mockReturnValue(mockRedisClientInstance);
   });
 
   describe('getRedisClient', () => {

redisinsight/api/src/modules/redis/redis-consumer.abstract.service.ts

@@ -143,14 +143,13 @@ export abstract class RedisConsumerAbstractService implements IRedisConsumer {
         instanceDto,
         connectionName,
       );
-      this.redisService.setClientInstance(
+      return this.redisService.setClientInstance(
         {
           ...clientMetadata,
           context: clientMetadata.context || this.consumer,
         },
         client,
-      );
-      return client;
+      )?.client;
     } catch (error) {
       throw catchRedisConnectionError(error, instanceDto);
     }

redisinsight/api/src/modules/redis/redis.service.spec.ts

@@ -121,29 +121,32 @@ describe('RedisService', () => {
 
       const result = service.setClientInstance(mockRedisClientInstance1.clientMetadata, mockIORedisClient);
 
-      expect(result).toEqual(1);
+      expect(result.client).toEqual(mockIORedisClient);
       expect(service.clients.size).toEqual(1);
     });
 
-    it('should replace existing client and update last used time', async () => {
+    it('should use existing client instead of replacing with new one', async () => {
       expect(service.clients.size).toEqual(0);
 
-      expect(service.setClientInstance(mockRedisClientInstance1.clientMetadata, mockIORedisClient)).toEqual(1);
+      expect(service.setClientInstance(mockRedisClientInstance1.clientMetadata, mockIORedisClient).client)
+        .toEqual(mockIORedisClient);
       expect(service.clients.size).toEqual(1);
 
-      const [justAddedClient] = [...service.clients.values()];
+      let [justAddedClient] = [...service.clients.values()];
+      justAddedClient = { ...justAddedClient };
 
       // sleep
       await new Promise((res) => setTimeout(res, 100));
 
-      expect(service.setClientInstance(mockRedisClientInstance1.clientMetadata, mockIORedisSentinel)).toEqual(0);
+      expect(service.setClientInstance(mockRedisClientInstance1.clientMetadata, mockIORedisSentinel).client)
+        .toEqual(mockIORedisClient);
       expect(service.clients.size).toEqual(1);
 
-      const [overwrittenClient] = [...service.clients.values()];
-      expect(overwrittenClient.clientMetadata).toEqual(justAddedClient.clientMetadata);
-      expect(overwrittenClient.id).toEqual(justAddedClient.id);
-      expect(overwrittenClient.client).not.toEqual(justAddedClient.client);
-      expect(overwrittenClient.lastTimeUsed).toBeGreaterThan(justAddedClient.lastTimeUsed);
+      const [newClient] = [...service.clients.values()];
+      expect(newClient.clientMetadata).toEqual(justAddedClient.clientMetadata);
+      expect(newClient.id).toEqual(justAddedClient.id);
+      expect(newClient.client).toEqual(justAddedClient.client);
+      expect(newClient.lastTimeUsed).toBeGreaterThan(justAddedClient.lastTimeUsed);
     });
   });
 

redisinsight/api/src/modules/redis/redis.service.ts

@@ -62,7 +62,7 @@ export class RedisService implements OnModuleDestroy {
     return found;
   }
 
-  public setClientInstance(clientMetadata: ClientMetadata, client): 0 | 1 {
+  public setClientInstance(clientMetadata: ClientMetadata, client): IRedisClientInstance {
     const metadata = RedisService.prepareClientMetadata(clientMetadata);
 
     const id = RedisService.generateId(metadata);
@@ -76,22 +76,18 @@ export class RedisService implements OnModuleDestroy {
     };
 
     if (found) {
-      // workaround for concurrent requests.
-      // At first try to gracefully close the connection using quit
-      try {
-        found.client.quit();
-      } catch (e) {
-        found.client.disconnect();
+      if (this.isClientConnected(found.client)) {
+        found.lastTimeUsed = Date.now();
+        client.disconnect();
+        return found;
       }
 
-      this.clients.delete(id);
-      this.clients.set(id, clientInstance);
-      return 0; // todo: investigate why we need to distinguish between 1 | 0
+      found.client.disconnect();
     }
 
     this.clients.set(id, clientInstance);
 
-    return 1;
+    return clientInstance;
   }
 
   public removeClientInstance(clientMetadata: ClientMetadata): number {