Merge pull request #69 from codota/boaz-rolling

Fix consecutive timeouts & restarts

src/main/java/com/tabnine/binary/BinaryRequestFacade.java

@@ -2,8 +2,9 @@
 
 import com.intellij.openapi.diagnostic.Logger;
 import com.intellij.util.concurrency.AppExecutorUtil;
+import com.tabnine.binary.exceptions.BinaryCannotRecoverException;
 import com.tabnine.binary.exceptions.TabNineDeadException;
-import com.tabnine.binary.exceptions.TooManyConsecutiveRestartsException;
+import com.tabnine.binary.exceptions.TabNineInvalidResponseException;
 import org.jetbrains.annotations.Nullable;
 
 import java.util.Queue;
@@ -16,10 +17,10 @@ public class BinaryRequestFacade {
     private final Queue<Future<?>> activeRequests = new ConcurrentLinkedQueue<>();
     private final AtomicInteger consecutiveRestarts = new AtomicInteger(0);
     private final AtomicInteger consecutiveTimeouts = new AtomicInteger(0);
-    private final TabNineGateway process;
+    private final TabNineGateway tabNineGateway;
 
-    public BinaryRequestFacade(TabNineGateway process) {
-        this.process = process;
+    public BinaryRequestFacade(TabNineGateway tabNineGateway) {
+        this.tabNineGateway = tabNineGateway;
     }
 
     public <R extends BinaryResponse> R executeRequest(BinaryRequest<R> req) {
@@ -28,34 +29,22 @@ public <R extends BinaryResponse> R executeRequest(BinaryRequest<R> req) {
 
     @Nullable
     public <R extends BinaryResponse> R executeRequest(BinaryRequest<R> req, int timeoutMillis) {
-        if (process.isStarting()) {
+        if (tabNineGateway.isStarting()) {
             Logger.getInstance(getClass()).info("Can't get completions because TabNine process is not started yet.");
 
             return null;
         }
+
         try {
-            Future<R> request = AppExecutorUtil.getAppExecutorService().submit(() -> {
-                try {
-                    return process.request(req);
-                } catch (TabNineDeadException e) {
-                    Logger.getInstance(getClass()).warn("TabNine is in invalid state, it is being restarted.", e);
-
-                    if (consecutiveRestarts.incrementAndGet() > CONSECUTIVE_RESTART_THRESHOLD) {
-                        Logger.getInstance(getClass()).error("Tabnine is not able to function properly. Contact [email protected]", new TooManyConsecutiveRestartsException());
-                        consecutiveRestarts.set(0);
-                    } else {
-                        restartBinary();
-                    }
-
-                    return null;
-                } finally {
-                    activeRequests.removeIf(Future::isDone);
-                }
-            });
-
-            activeRequests.add(request);
-
-            return request.get(timeoutMillis, TimeUnit.MILLISECONDS);
+            Future<R> requestFuture = AppExecutorUtil.getAppExecutorService().submit(() -> executeBoundlessRequest(req));
+
+            activeRequests.add(requestFuture);
+
+            R result = requestFuture.get(timeoutMillis, TimeUnit.MILLISECONDS);
+
+            consecutiveTimeouts.set(0);
+
+            return result;
         } catch (TimeoutException e) {
             Logger.getInstance(getClass()).info("TabNine's response timed out.");
 
@@ -64,18 +53,52 @@ public <R extends BinaryResponse> R executeRequest(BinaryRequest<R> req, int tim
                 consecutiveTimeouts.set(0);
                 restartBinary();
             }
+        } catch (ExecutionException e) {
+            if(e.getCause().getCause() instanceof BinaryCannotRecoverException) {
+                throw (BinaryCannotRecoverException) e.getCause().getCause();
+            }
+
+            Logger.getInstance(getClass()).warn("TabNine's threw an unknown error during request.", e);
         } catch (CancellationException e) {
             // This is ok. Nothing needs to be done.
-        } catch (Throwable t) {
-            Logger.getInstance(getClass()).error("TabNine's threw an unknown error.", t);
+        } catch (Exception e) {
+            Logger.getInstance(getClass()).error("TabNine's threw an unknown error.", e);
         }
 
         return null;
     }
 
+    @Nullable
+    private <R extends BinaryResponse> R executeBoundlessRequest(BinaryRequest<R> req) {
+        try {
+            R result = tabNineGateway.request(req);
+
+            consecutiveRestarts.set(0);
+
+            return result;
+        } catch (TabNineDeadException e) {
+            Logger.getInstance(getClass()).warn("TabNine is in invalid state, it is being restarted.", e);
+
+            if (consecutiveRestarts.incrementAndGet() > CONSECUTIVE_RESTART_THRESHOLD) {
+                // NOTICE: In the production version of IntelliJ, logging an error kills the plugin. So this is similar to exit(1);
+                Logger.getInstance(getClass()).error("Tabnine is not able to function properly. Contact [email protected]", new BinaryCannotRecoverException());
+            } else {
+                restartBinary();
+            }
+
+            return null;
+        } catch (TabNineInvalidResponseException e) {
+            Logger.getInstance(getClass()).warn(e);
+
+            return null;
+        } finally {
+            activeRequests.removeIf(Future::isDone);
+        }
+    }
+
     private void restartBinary() {
         this.activeRequests.forEach(f -> f.cancel(true));
         this.activeRequests.clear();
-        this.process.restart();
+        this.tabNineGateway.restart();
     }
 }

src/main/java/com/tabnine/binary/TabNineGateway.java

@@ -72,7 +72,7 @@ private void startBinary(SideEffectExecutor onStartBinaryAttempt) {
      * @throws TabNineDeadException if the result from the process was invalid multiple times.
      */
     @Nullable
-    public synchronized <R extends BinaryResponse> R request(BinaryRequest<R> request) throws TabNineDeadException {
+    public synchronized <R extends BinaryResponse> R request(BinaryRequest<R> request) throws TabNineDeadException, TabNineInvalidResponseException {
         if (TabNineProcessFacade.isDead()) {
             throw new TabNineDeadException("Binary is dead");
         }
@@ -85,10 +85,6 @@ public synchronized <R extends BinaryResponse> R request(BinaryRequest<R> reques
             Logger.getInstance(getClass()).warn("Exception communicating with the binary!", e);
 
             throw new TabNineDeadException(e);
-        } catch (TabNineInvalidResponseException e) {
-            Logger.getInstance(getClass()).warn(e);
-
-            return null;
         }
     }
 

src/main/java/com/tabnine/binary/exceptions/BinaryCannotRecoverException.java

@@ -0,0 +1,7 @@
+package com.tabnine.binary.exceptions;
+
+public class BinaryCannotRecoverException extends RuntimeException {
+    public BinaryCannotRecoverException() {
+        super();
+    }
+}

src/main/java/com/tabnine/prediction/CompletionFacade.java

@@ -6,6 +6,7 @@
 import com.intellij.openapi.progress.ProgressManager;
 import com.intellij.openapi.util.TextRange;
 import com.tabnine.binary.BinaryRequestFacade;
+import com.tabnine.binary.exceptions.BinaryCannotRecoverException;
 import com.tabnine.binary.requests.autocomplete.AutocompleteRequest;
 import com.tabnine.binary.requests.autocomplete.AutocompleteResponse;
 
@@ -39,6 +40,8 @@ public AutocompleteResponse retrieveCompletions(CompletionParameters parameters)
 
                 return binaryRequestFacade.executeRequest(req);
             }, ProgressManager.getInstance().getProgressIndicator());
+        } catch (BinaryCannotRecoverException e) {
+            throw e;
         } catch (Exception e) {
             return null;
         }

src/test/java/com/tabnine/integration/PredictionBehaviourIntegrationTests.java

@@ -2,10 +2,14 @@
 
 import org.junit.Test;
 
+import java.util.concurrent.ExecutionException;
+import java.util.stream.Stream;
+
+import static com.tabnine.testutils.BadResultsUtils.enoughBadResultsToCauseADeath;
+import static com.tabnine.testutils.BadResultsUtils.overThresholdBadResultsWithAGoodResultInBetween;
 import static com.tabnine.testutils.TabnineMatchers.lookupBuilder;
 import static com.tabnine.testutils.TabnineMatchers.lookupElement;
-import static com.tabnine.testutils.TestData.A_PREDICTION_RESULT;
-import static com.tabnine.testutils.TestData.A_REQUEST_TO_TABNINE_BINARY;
+import static com.tabnine.testutils.TestData.*;
 import static org.hamcrest.Matchers.array;
 import static org.junit.Assert.assertThat;
 import static org.mockito.Mockito.verify;
@@ -31,4 +35,32 @@ public void givenAFileWhenCompletionFiredThenResponseFromBinaryParsedCorrectlyTo
                 lookupElement("return result;")
         ));
     }
+
+    @Test
+    public void givenInvalidCompletionsThatAreNotConsecutiveThanPluginIsNotDead() throws Exception {
+        String[] enoughResultsToCauseDeathIfWerentForGoodResultBetweenEndingWithAGoodResult = Stream.concat(overThresholdBadResultsWithAGoodResultInBetween(), Stream.of(A_PREDICTION_RESULT)).toArray(String[]::new);
+        when(tabNineBinaryMock.readRawResponse()).thenReturn(INVALID_RESULT, enoughResultsToCauseDeathIfWerentForGoodResultBetweenEndingWithAGoodResult);
+
+        for (int i = 0; i < enoughResultsToCauseDeathIfWerentForGoodResultBetweenEndingWithAGoodResult.length; i++) {
+            myFixture.completeBasic();
+        }
+
+        assertThat(myFixture.completeBasic(), array(
+                lookupBuilder("hello"),
+                lookupElement("return result"),
+                lookupElement("return result;")
+        ));
+    }
+
+    @Test
+    public void givenMoreConsecutiveInvalidCompletionsThanThresholdThenPluginDies() throws Exception {
+        String[] badResults = enoughBadResultsToCauseADeath().toArray(String[]::new);
+        when(tabNineBinaryMock.readRawResponse()).thenReturn(INVALID_RESULT, badResults);
+
+        assertThrows(ExecutionException.class, () -> {
+            for (int i = 0; i < badResults.length; i++) {
+                myFixture.completeBasic();
+            }
+        });
+    }
 }

src/test/java/com/tabnine/integration/PredictionTimeoutIntegrationTests.java

@@ -13,8 +13,7 @@
 import static com.tabnine.testutils.TestData.*;
 import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.assertThat;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.*;
 
 public class PredictionTimeoutIntegrationTests extends MockedBinaryCompletionTestCase {
     @Test
@@ -98,4 +97,39 @@ public void givenConsecutivesTimeOutsCompletionWhenCompletionThenPreviousResultI
                 lookupElement("test")
         ));
     }
+
+    @Test
+    public void givenCompletionTimeOutsButNotConsecutiveWhenCompletionThenRestartIsNotHappening() throws Exception {
+        AtomicInteger index = new AtomicInteger();
+        when(tabNineBinaryMock.readRawResponse()).then((invocation) -> {
+            int currentIndex = index.getAndIncrement();
+
+            if(currentIndex == INDEX_OF_SOME_VALID_RESULT_BETWEEN_TIMEOUTS) {
+                return A_PREDICTION_RESULT;
+            }
+
+            if(currentIndex < CONSECUTIVE_TIMEOUTS_THRESHOLD + OVERFLOW) {
+                Thread.sleep(COMPLETION_TIME_THRESHOLD + EPSILON);
+
+                return A_PREDICTION_RESULT;
+            }
+
+            Thread.sleep(EPSILON);
+            return SECOND_PREDICTION_RESULT;
+        });
+
+        for(int i = 0; i < CONSECUTIVE_TIMEOUTS_THRESHOLD + OVERFLOW; i++) {
+            if(i != INDEX_OF_SOME_VALID_RESULT_BETWEEN_TIMEOUTS) {
+                assertThat(myFixture.completeBasic(), nullValue());
+            } else {
+                myFixture.completeBasic();
+            }
+        }
+
+        assertThat(myFixture.completeBasic(), array(
+                lookupBuilder("hello"),
+                lookupElement("test")
+        ));
+        verify(tabNineBinaryMock, never()).restart();
+    }
 }

src/test/java/com/tabnine/testutils/BadResultsUtils.java

@@ -0,0 +1,37 @@
+package com.tabnine.testutils;
+
+import org.jetbrains.annotations.NotNull;
+
+import java.util.stream.IntStream;
+import java.util.stream.Stream;
+
+import static com.tabnine.general.StaticConfig.CONSECUTIVE_RESTART_THRESHOLD;
+import static com.tabnine.general.StaticConfig.ILLEGAL_RESPONSE_THRESHOLD;
+import static com.tabnine.testutils.TestData.*;
+
+public class BadResultsUtils {
+    @NotNull
+    public static Stream<String> overThresholdBadResultsWithAGoodResultInBetween() {
+        Stream<String> results = Stream.concat(enoughBadResultsToCauseARestart(), Stream.of(A_PREDICTION_RESULT));
+
+        for(int i = 0; i < CONSECUTIVE_RESTART_THRESHOLD; i++) {
+            results = Stream.concat(results, enoughBadResultsToCauseARestart());
+        }
+
+        return results;
+    }
+    @NotNull
+    public static Stream<String> enoughBadResultsToCauseADeath() {
+        Stream<String> results = enoughBadResultsToCauseARestart();
+
+        for(int i = 0; i < CONSECUTIVE_RESTART_THRESHOLD; i++) {
+            results = Stream.concat(results, enoughBadResultsToCauseARestart());
+        }
+
+        return results;
+    }
+
+    private static Stream<String> enoughBadResultsToCauseARestart() {
+        return IntStream.range(0, ILLEGAL_RESPONSE_THRESHOLD + OVERFLOW).mapToObj(i -> INVALID_RESULT);
+    }
+}

src/test/java/com/tabnine/testutils/TestData.java

@@ -37,6 +37,7 @@ public class TestData {
     public static final String NO_EXTENSION_STATE_REQUEST = "{\"request\":{\"SetState\":{\"state_type\":{\"Selection\":{\"language\":\"undefined\",\"length\":13,\"origin\":\"LOCAL\",\"net_length\":13,\"strength\":\"11%\",\"index\":1,\"line_prefix_length\":5,\"line_net_prefix_length\":5,\"line_suffix_length\":0,\"num_of_suggestions\":2,\"num_of_vanilla_suggestions\":0,\"num_of_deep_local_suggestions\":2,\"num_of_deep_cloud_suggestions\":0,\"num_of_lsp_suggestions\":0,\"suggestions\":[{\"length\":13,\"strength\":\"11%\",\"origin\":\"LOCAL\"},{\"length\":14,\"strength\":\" 7%\",\"origin\":\"LOCAL\"}]}}}},\"version\":\"2.0.2\"}\n";
     public static final String SET_STATE_RESPONSE = "{\"result\":\"Done\"}";
     public static final String NULL_RESULT = "null";
+    public static final int INDEX_OF_SOME_VALID_RESULT_BETWEEN_TIMEOUTS = 2;
 
     public static byte[] binaryContentSized(int size) {
         return new byte[size];