SONAR-15918 improve connections management of SonarLintClients

4 files changed, 38 insertions, 18 deletions

diff --git a/server/sonar-webserver-pushapi/src/main/java/org/sonar/server/pushapi/ServerPushClient.java b/server/sonar-webserver-pushapi/src/main/java/org/sonar/server/pushapi/ServerPushClient.java
index 6d8b552e24a..a2004270b23 100644
--- a/server/sonar-webserver-pushapi/src/main/java/org/sonar/server/pushapi/ServerPushClient.java
+++ b/server/sonar-webserver-pushapi/src/main/java/org/sonar/server/pushapi/ServerPushClient.java
@@ -85,7 +85,7 @@ public abstract class ServerPushClient {
   private void handleIOException(IOException e) {
     String remoteAddr = asyncContext.getRequest().getRemoteAddr();
     LOG.info(String.format("The server push client %s gone without notice, closing the connection (%s)", remoteAddr, e.getMessage()));
-    close();
+    throw new IllegalStateException(e.getMessage());
   }
 
   public synchronized void close() {
diff --git a/server/sonar-webserver-pushapi/src/main/java/org/sonar/server/pushapi/sonarlint/SonarLintClientsRegistry.java b/server/sonar-webserver-pushapi/src/main/java/org/sonar/server/pushapi/sonarlint/SonarLintClientsRegistry.java
index 008fdb0c3f9..6bdc3b7b580 100644
--- a/server/sonar-webserver-pushapi/src/main/java/org/sonar/server/pushapi/sonarlint/SonarLintClientsRegistry.java
+++ b/server/sonar-webserver-pushapi/src/main/java/org/sonar/server/pushapi/sonarlint/SonarLintClientsRegistry.java
@@ -37,8 +37,8 @@ import org.sonar.core.util.ParamChange;
 import org.sonar.core.util.RuleActivationListener;
 import org.sonar.core.util.RuleChange;
 import org.sonar.core.util.RuleSetChangeEvent;
-import org.sonar.server.pushapi.qualityprofile.RuleActivatorEventsDistributor;
 import org.sonar.server.exceptions.ForbiddenException;
+import org.sonar.server.pushapi.qualityprofile.RuleActivatorEventsDistributor;
 
 import static java.util.Arrays.asList;
 
@@ -47,21 +47,19 @@ public class SonarLintClientsRegistry implements RuleActivationListener {
 
   private static final Logger LOG = Loggers.get(SonarLintClientsRegistry.class);
 
-  private final RuleActivatorEventsDistributor ruleActivatorEventsDistributor;
   private final SonarLintClientPermissionsValidator sonarLintClientPermissionsValidator;
   private final List<SonarLintClient> clients = new CopyOnWriteArrayList<>();
 
   public SonarLintClientsRegistry(RuleActivatorEventsDistributor ruleActivatorEventsDistributor, SonarLintClientPermissionsValidator permissionsValidator) {
-    this.ruleActivatorEventsDistributor = ruleActivatorEventsDistributor;
     this.sonarLintClientPermissionsValidator = permissionsValidator;
+
+    ruleActivatorEventsDistributor.subscribe(this);
   }
 
   public void registerClient(SonarLintClient sonarLintClient) {
     clients.add(sonarLintClient);
     sonarLintClient.scheduleHeartbeat();
     sonarLintClient.addListener(new SonarLintClientEventsListener(sonarLintClient));
-    ruleActivatorEventsDistributor.subscribe(this);
-
     LOG.debug("Registering new SonarLint client");
   }
 
@@ -77,7 +75,6 @@ public class SonarLintClientsRegistry implements RuleActivationListener {
 
   @Override
   public void listen(RuleSetChangeEvent ruleChangeEvent) {
-    LOG.info("Generating a RuleSetChangeEvent");
     broadcastMessage(ruleChangeEvent, getFilterForEvent(ruleChangeEvent));
   }
 
@@ -101,9 +98,11 @@ public class SonarLintClientsRegistry implements RuleActivationListener {
         String jsonString = getJSONString(personalizedEvent);
         c.writeAndFlush(jsonString);
       } catch (ForbiddenException forbiddenException) {
+        LOG.debug("Client is no longer authenticated: " + forbiddenException.getMessage());
         unregisterClient(c);
-      } catch (IOException e) {
+      } catch (IllegalStateException | IOException e) {
         LOG.error("Unable to send message to a client: " + e.getMessage());
+        unregisterClient(c);
       }
     });
   }
diff --git a/server/sonar-webserver-pushapi/src/test/java/org/sonar/server/pushapi/ServerPushClientTest.java b/server/sonar-webserver-pushapi/src/test/java/org/sonar/server/pushapi/ServerPushClientTest.java
index e7eca1cf790..462d4a849fe 100644
--- a/server/sonar-webserver-pushapi/src/test/java/org/sonar/server/pushapi/ServerPushClientTest.java
+++ b/server/sonar-webserver-pushapi/src/test/java/org/sonar/server/pushapi/ServerPushClientTest.java
@@ -34,6 +34,7 @@ import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mockito;
 
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.doThrow;
@@ -123,20 +124,18 @@ public class ServerPushClientTest {
     when(executorService.schedule(any(HeartbeatTask.class), anyLong(), any(TimeUnit.class))).thenReturn(task);
     underTest.scheduleHeartbeat();
 
-    underTest.write('a');
-
-    verify(asyncContext).complete();
+    assertThatThrownBy(() -> underTest.write('a'))
+      .isInstanceOf(IllegalStateException.class);
   }
 
   @Test
-  public void flush_exceptionCausesConnectionToClose() throws IOException {
+  public void flush_exceptionIsPropagated() throws IOException {
     when(servletResponse.getOutputStream()).thenThrow(new IOException("mock exception"));
     when(executorService.schedule(any(HeartbeatTask.class), anyLong(), any(TimeUnit.class))).thenReturn(task);
     underTest.scheduleHeartbeat();
 
-    underTest.flush();
-
-    verify(asyncContext).complete();
+    assertThatThrownBy(underTest::flush)
+      .isInstanceOf(IllegalStateException.class);
   }
 
   @Test
diff --git a/server/sonar-webserver-pushapi/src/test/java/org/sonar/server/pushapi/sonarlint/SonarLintClientsRegistryTest.java b/server/sonar-webserver-pushapi/src/test/java/org/sonar/server/pushapi/sonarlint/SonarLintClientsRegistryTest.java
index 351ff62fb3e..208fa45715f 100644
--- a/server/sonar-webserver-pushapi/src/test/java/org/sonar/server/pushapi/sonarlint/SonarLintClientsRegistryTest.java
+++ b/server/sonar-webserver-pushapi/src/test/java/org/sonar/server/pushapi/sonarlint/SonarLintClientsRegistryTest.java
@@ -39,6 +39,7 @@ import static org.mockito.ArgumentMatchers.anySet;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
@@ -64,8 +65,8 @@ public class SonarLintClientsRegistryTest {
   }
 
   @Test
-  public void registerClientAndUnregister_changesNumberOfClients() {
-    SonarLintClient sonarLintClient = new SonarLintClient(defaultAsyncContext, exampleKeys, languageKeys, USER_UUID);
+  public void registerClientAndUnregister_changesNumberOfClients_andClosesClient() {
+    SonarLintClient sonarLintClient = mock(SonarLintClient.class);
 
     underTest.registerClient(sonarLintClient);
 
@@ -74,6 +75,7 @@ public class SonarLintClientsRegistryTest {
     underTest.unregisterClient(sonarLintClient);
 
     assertThat(underTest.countConnectedClients()).isZero();
+    verify(sonarLintClient).close();
   }
 
   @Test
@@ -168,6 +170,27 @@ public class SonarLintClientsRegistryTest {
     verify(sonarLintClient).close();
   }
 
+  @Test
+  public void listen_givenUnregisteredClient_closeConnection() throws IOException {
+    RuleChange javaRuleChange = createRuleChange();
+    RuleChange[] activatedRules = {};
+    RuleChange[] deactivatedRules = {javaRuleChange};
+    RuleSetChangeEvent ruleChangeEvent = new RuleSetChangeEvent(exampleKeys.toArray(String[]::new), activatedRules, deactivatedRules);
+
+    SonarLintClient sonarLintClient = createSampleSLClient();
+    underTest.registerClient(sonarLintClient);
+    doThrow(new IOException("Broken pipe")).when(sonarLintClient).writeAndFlush(anyString());
+
+    underTest.listen(ruleChangeEvent);
+
+    underTest.registerClient(sonarLintClient);
+    doThrow(new IllegalStateException("Things went wrong")).when(sonarLintClient).writeAndFlush(anyString());
+
+    underTest.listen(ruleChangeEvent);
+
+    verify(sonarLintClient, times(2)).close();
+  }
+
   private SonarLintClient createSampleSLClient() {
     SonarLintClient mock = mock(SonarLintClient.class);
     when(mock.getLanguages()).thenReturn(Set.of("java"));
@@ -185,5 +208,4 @@ public class SonarLintClientsRegistryTest {
     javaRule.setKey("rule-key");
     return javaRule;
   }
-
 }