From 0fd6351a769ef75692ff53e869425bed6a3b82ce Mon Sep 17 00:00:00 2001
From: Trisha Anand <trisha@appsmith.com>
Date: Tue, 26 Nov 2019 11:34:27 +0000
Subject: [PATCH] Added timeout on the plugin execution. Next TODO : Make the
 timeout duration configurable

---
 .../external/models/ActionConfiguration.java         |  2 ++
 .../appsmith/server/exceptions/AppsmithError.java    |  3 ++-
 .../server/exceptions/GlobalExceptionHandler.java    | 12 +++++++++++-
 .../appsmith/server/services/ActionServiceImpl.java  |  9 ++++++---
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/ActionConfiguration.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/ActionConfiguration.java
index b64868cb63..75ee7dffac 100644
--- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/ActionConfiguration.java
+++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/ActionConfiguration.java
@@ -25,6 +25,8 @@ public class ActionConfiguration {
      * action execution.
      */
 
+    int timeoutInMillisecond = 10000;
+
     // API fields
     String path;
     List<Property> headers;
diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java
index acfef751e4..c0533514e0 100644
--- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java
+++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java
@@ -31,7 +31,8 @@ public enum AppsmithError {
     INTERNAL_SERVER_ERROR(500, 5000, "Internal server error while processing request"),
     REPOSITORY_SAVE_FAILED(500, 5001, "Repository save failed"),
     PLUGIN_INSTALLATION_FAILED_DOWNLOAD_ERROR(500, 5002, "Due to error in downloading the plugin from remote repository, plugin installation has failed. Check the jar location and try again"),
-    PLUGIN_RUN_FAILED(500, 5003, "Plugin execution failed with error {0}");
+    PLUGIN_RUN_FAILED(500, 5003, "Plugin execution failed with error {0}"),
+    PLUGIN_EXECUTION_TIMEOUT(504, 5040, "Plugin Execution exceeded the maximum allowed time. Please increase the timeout in your action settings or check your backend action endpoint");
 
 
     private Integer httpErrorCode;
diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java
index 64c6c5491f..8f0959e728 100644
--- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java
+++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java
@@ -48,7 +48,7 @@ public class GlobalExceptionHandler {
 
     @ExceptionHandler
     @ResponseBody
-    public Mono<ResponseDTO<ErrorDTO>> catchException(org.springframework.dao.DuplicateKeyException e, ServerWebExchange exchange) {
+    public Mono<ResponseDTO<ErrorDTO>> catchDuplicateKeyException(org.springframework.dao.DuplicateKeyException e, ServerWebExchange exchange) {
         exchange.getResponse().setStatusCode(HttpStatus.BAD_REQUEST);
         log.error("", e);
         rollbar.log(e);
@@ -56,6 +56,16 @@ public class GlobalExceptionHandler {
                 AppsmithError.DUPLICATE_KEY.getMessage())));
     }
 
+    @ExceptionHandler
+    @ResponseBody
+    public Mono<ResponseDTO<ErrorDTO>> catchTimeoutException(java.util.concurrent.TimeoutException e, ServerWebExchange exchange) {
+        exchange.getResponse().setStatusCode(HttpStatus.GATEWAY_TIMEOUT);
+        log.error("", e);
+        rollbar.log(e);
+        return Mono.just(new ResponseDTO<>(HttpStatus.GATEWAY_TIMEOUT.value(), new ErrorDTO(AppsmithError.PLUGIN_EXECUTION_TIMEOUT.getHttpErrorCode(),
+                AppsmithError.PLUGIN_EXECUTION_TIMEOUT.getMessage())));
+    }
+
     /**
      * This function catches the generic Exception class and is meant to be a catch all to ensure that we don't leak
      * any information to the client. Ideally, the function #catchAppsmithException should be used
diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ActionServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ActionServiceImpl.java
index c843efe9c9..be38c54984 100644
--- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ActionServiceImpl.java
+++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ActionServiceImpl.java
@@ -37,6 +37,7 @@ import javax.validation.constraints.NotNull;
 import java.io.StringReader;
 import java.io.StringWriter;
 import java.io.Writer;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
@@ -296,6 +297,7 @@ public class ActionServiceImpl extends BaseService<ActionRepository, Action, Str
     @Override
     public Mono<ActionExecutionResult> executeAction(ExecuteActionDTO executeActionDTO) {
         Action actionFromDto = executeActionDTO.getAction();
+        final int[] timeoutDuration = new int[1];
 
         // 1. Validate input parameters which are required for mustache replacements
         List<Param> params = executeActionDTO.getParams();
@@ -365,7 +367,7 @@ public class ActionServiceImpl extends BaseService<ActionRepository, Action, Str
                                 .getParams()
                                 .stream()
                                 .collect(Collectors.toMap(Param::getKey, Param::getValue,
-                                        // Incase there's a conflict, we pick the older value
+                                        // In case of a conflict, we pick the older value
                                         (oldValue, newValue) -> oldValue)
                                 );
                         datasourceConfiguration = (DatasourceConfiguration) variableSubstitution(datasource.getDatasourceConfiguration(), replaceParamsMap);
@@ -374,6 +376,7 @@ public class ActionServiceImpl extends BaseService<ActionRepository, Action, Str
                         datasourceConfiguration = datasource.getDatasourceConfiguration();
                         actionConfiguration = action.getActionConfiguration();
                     }
+                    timeoutDuration[0] = actionConfiguration.getTimeoutInMillisecond();
                     return datasourceContextService
                             .getDatasourceContext(datasource)
                             //Now that we have the context (connection details, execute the action
@@ -382,8 +385,8 @@ public class ActionServiceImpl extends BaseService<ActionRepository, Action, Str
                                     datasourceConfiguration,
                                     actionConfiguration));
                 }))
-//                .onErrorResume(e -> Mono.error(new AppsmithException(AppsmithError.PLUGIN_RUN_FAILED, e.getMessage())))
-                .flatMap(obj -> obj);
+                .flatMap(obj -> obj)
+                .timeout(Duration.ofMillis(timeoutDuration[0]));
     }
 
     @Override