From 950026dabca8ceedc127dca65a950528f52384d8 Mon Sep 17 00:00:00 2001 From: Roly Gutierrez Date: Fri, 23 Jun 2023 17:42:36 -0400 Subject: [PATCH 01/12] FOUR-8993 Use the queue for retries instead of sleeping and retrying in the same job --- ProcessMaker/Jobs/ClosureJob.php | 8 ++++++++ ProcessMaker/Jobs/ClosureJobInterface.php | 21 +++++++++++++++++++++ ProcessMaker/Models/Script.php | 18 ++++++++++++------ 3 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 ProcessMaker/Jobs/ClosureJob.php create mode 100644 ProcessMaker/Jobs/ClosureJobInterface.php diff --git a/ProcessMaker/Jobs/ClosureJob.php b/ProcessMaker/Jobs/ClosureJob.php new file mode 100644 index 0000000000..2a96581ab2 --- /dev/null +++ b/ProcessMaker/Jobs/ClosureJob.php @@ -0,0 +1,8 @@ +retryWaitTime()} seconds before retrying."); - sleep($this->retryWaitTime()); + $closure = function () use ($data, $config, $tokenId, $errorHandling) { + Log::info('Re-running the script'); + $this->runScript($data, $config, $tokenId, $errorHandling);//todo: it is important to return the correct data. + Log::info('The script completed successfully'); + }; + + $retryWaitTime = $this->retryWaitTime(); + Log::info("Waiting {$retryWaitTime} seconds before retrying."); $this->attemptedRetries++; - Log::info('Re-running the script'); - $result = $this->runScript($data, $config, $tokenId, $errorHandling); - Log::info('The script completed successfully'); + Queue::later($retryWaitTime, new ClosureJob($closure)); - return $result; + return ['output'];//todo: it is important to return the correct data. } else { $this->sendExecutionErrorNotification($e->getMessage(), $tokenId, $errorHandling); throw $e; From 71d7ff7729263e809ea530631e7ef8565cee2489 Mon Sep 17 00:00:00 2001 From: Roly Gutierrez Date: Wed, 28 Jun 2023 16:16:52 -0400 Subject: [PATCH 02/12] FOUR-8993 new improvements --- ProcessMaker/Exception/RetryableException.php | 17 +++ ProcessMaker/Jobs/BpmnAction.php | 8 +- ProcessMaker/Jobs/ClosureJob.php | 8 -- ProcessMaker/Jobs/ClosureJobInterface.php | 21 ---- ProcessMaker/Jobs/RunScriptTask.php | 57 +++++++-- ProcessMaker/Jobs/RunServiceTask.php | 3 +- ProcessMaker/Models/ProcessRequest.php | 10 +- ProcessMaker/Models/Script.php | 116 +----------------- .../Models/ScriptDockerBindingFilesTrait.php | 2 +- ProcessMaker/Traits/MakeHttpRequests.php | 38 ++---- resources/lang/en.json | 4 +- .../views/processes/scripts/edit.blade.php | 4 +- 12 files changed, 95 insertions(+), 193 deletions(-) create mode 100644 ProcessMaker/Exception/RetryableException.php delete mode 100644 ProcessMaker/Jobs/ClosureJob.php delete mode 100644 ProcessMaker/Jobs/ClosureJobInterface.php diff --git a/ProcessMaker/Exception/RetryableException.php b/ProcessMaker/Exception/RetryableException.php new file mode 100644 index 0000000000..cf179a6f8c --- /dev/null +++ b/ProcessMaker/Exception/RetryableException.php @@ -0,0 +1,17 @@ +engine->runToNextState(); + } catch (RetryableException $e) { + Log::info('Re-Dispatching. Attempts: ' . $this->attempts() . ', Wait time: ' . $e->retry_wait_time); + $this->release($e->retry_wait_time); } catch (Throwable $exception) { - Log::error($exception->getMessage()); + Log::error('%% ' . $exception->getMessage()); // Change the Request to error status $request = !$this->instance && $this instanceof StartEvent ? $response : $this->instance; if ($request) { @@ -80,7 +84,7 @@ public function handle() * * @return array */ - private function loadContext() + protected function loadContext() { //Load the process definition if (isset($this->instanceId)) { diff --git a/ProcessMaker/Jobs/ClosureJob.php b/ProcessMaker/Jobs/ClosureJob.php deleted file mode 100644 index 2a96581ab2..0000000000 --- a/ProcessMaker/Jobs/ClosureJob.php +++ /dev/null @@ -1,8 +0,0 @@ -onQueue('bpmn'); $this->definitionsId = $definitions->getKey(); $this->instanceId = $instance->getKey(); $this->tokenId = $token->getKey(); $this->elementId = $token->getProperty('element_ref'); $this->data = $data; + + $this->tries = 4;// } /** @@ -60,10 +66,10 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e } $scriptRef = $element->getProperty('scriptRef'); $configuration = json_decode($element->getProperty('config'), true); - $errorHandling = json_decode($element->getProperty('errorHandling'), true); - if ($errorHandling === null) { - $errorHandling = []; - } + $errorHandling = json_decode($element->getProperty('errorHandling'), true) ?? []; + $errorHandling['attempts'] = $this->attempts(); + $errorHandling['retry_attempts'] = ctype_digit($errorHandling['retry_attempts']) ? intval($errorHandling['retry_attempts']) : 0; + $errorHandling['retry_wait_time'] = ctype_digit($errorHandling['retry_wait_time']) ? intval($errorHandling['retry_wait_time']) : 0; // Check to see if we've failed parsing. If so, let's convert to empty array. if ($configuration === null) { @@ -89,7 +95,7 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e $this->unlock(); $dataManager = new DataManager(); $data = $dataManager->getData($token); - $response = $script->runScript($data, $configuration, $token->getId(), $errorHandling); + $response = $script->runScript($data, $configuration, $token->getId()); $this->withUpdatedContext(function ($engine, $instance, $element, $processModel, $token) use ($response) { // Exit if the task was completed or closed @@ -117,8 +123,41 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e $token->setProperty('error', $error); $token->logError($exception, $element); - Log::error('Script failed: ' . $scriptRef . ' - ' . $exception->getMessage()); - Log::error($exception->getTraceAsString()); + Log::error('Script failed: ' . $scriptRef . ' - ');// . $exception->getMessage()); + //Log::error($exception->getTraceAsString()); + + if ($errorHandling['retry_attempts'] > 0) { + if ($this->attempts() <= $errorHandling['retry_attempts']) { + Log::info('Retry the runScript process. Attempt ' . $this->attempts() . ' of ' . $errorHandling['retry_attempts']); + throw new RetryableException($errorHandling['retry_wait_time']); + } + + $message = __('Script failed after :attempts total attempts', ['attempts' => $this->attempts()]); + $message = $message . "\n" . $exception->getMessage(); + + $this->sendExecutionErrorNotification($message, $token->getId(), $errorHandling); + if ($exception instanceof ScriptTimeoutException) { + throw new ScriptTimeoutException($message); + } + throw new ScriptException($message); + } else { + $this->sendExecutionErrorNotification($exception->getMessage(), $token->getId(), $errorHandling); + throw $exception; + } + } + } + + /** + * Send execution error notification. + */ + public function sendExecutionErrorNotification(string $message, string $tokenId, array $errorHandling) + { + $processRequestToken = ProcessRequestToken::find($tokenId); + if ($processRequestToken) { + $user = $processRequestToken->processRequest->processVersion->manager; + if ($user !== null) { + Notification::send($user, new ErrorExecutionNotification($processRequestToken, $message, $errorHandling)); + } } } diff --git a/ProcessMaker/Jobs/RunServiceTask.php b/ProcessMaker/Jobs/RunServiceTask.php index 3071adc163..c01c441d51 100644 --- a/ProcessMaker/Jobs/RunServiceTask.php +++ b/ProcessMaker/Jobs/RunServiceTask.php @@ -65,7 +65,8 @@ public function action(ProcessRequestToken $token = null, ServiceTaskInterface $ } $implementation = $element->getImplementation(); $configuration = json_decode($element->getProperty('config'), true); - $errorHandling = json_decode($element->getProperty('errorHandling'), true); + $errorHandling = json_decode($element->getProperty('errorHandling'), true) ?? []; + $errorHandling['attempts'] = $this->attempts(); // Check to see if we've failed parsing. If so, let's convert to empty array. if ($configuration === null) { diff --git a/ProcessMaker/Models/ProcessRequest.php b/ProcessMaker/Models/ProcessRequest.php index 03f8643a57..fe903c5890 100644 --- a/ProcessMaker/Models/ProcessRequest.php +++ b/ProcessMaker/Models/ProcessRequest.php @@ -549,10 +549,12 @@ public function logError(Throwable $exception, FlowElementInterface $element = n $errors[] = $error; $this->errors = $errors; $this->status = 'ERROR'; - \Log::error($exception); - if (!$this->isNonPersistent()) { - $this->save(); - } + // $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 5); + // \Log::info("TRACE", $trace); + // \Log::error($exception); + // if (!$this->isNonPersistent()) { + // $this->save(); + // } } public function childRequests() diff --git a/ProcessMaker/Models/Script.php b/ProcessMaker/Models/Script.php index f87f14c23b..fbdb933c51 100644 --- a/ProcessMaker/Models/Script.php +++ b/ProcessMaker/Models/Script.php @@ -2,21 +2,11 @@ namespace ProcessMaker\Models; -use Hamcrest\Type\IsNumeric; -use Illuminate\Support\Arr; -use Illuminate\Support\Facades\Log; -use Illuminate\Support\Facades\Notification; -use Illuminate\Support\Facades\Queue; use Illuminate\Validation\Rule; use ProcessMaker\Contracts\ScriptInterface; -use ProcessMaker\Exception\ScriptException; use ProcessMaker\Exception\ScriptLanguageNotSupported; -use ProcessMaker\Exception\ScriptTimeoutException; -use ProcessMaker\GenerateAccessToken; -use ProcessMaker\Jobs\ClosureJob; use ProcessMaker\Models\ScriptCategory; use ProcessMaker\Models\User; -use ProcessMaker\Notifications\ErrorExecutionNotification; use ProcessMaker\ScriptRunners\ScriptRunner; use ProcessMaker\Traits\Exportable; use ProcessMaker\Traits\HasCategories; @@ -86,14 +76,8 @@ class Script extends ProcessMakerModel implements ScriptInterface protected $casts = [ 'timeout' => 'integer', - 'retry_attempts' => 'integer', - 'retry_wait_time' => 'integer', ]; - private $errorHandling = []; - - private $attemptedRetries = 1; - /** * Override the default boot method to allow access to lifecycle hooks * @@ -131,8 +115,6 @@ public static function rules($existing = null) 'description' => 'required', 'run_as_user_id' => 'required', 'timeout' => 'integer|min:0|max:65535', - 'retry_attempts' => 'integer|min:0|max:65535', - 'retry_wait_time' => 'integer|min:0|max:65535', 'script_category_id' => [new CategoryRule($existing)], ]; } @@ -143,12 +125,11 @@ public static function rules($existing = null) * @param array $data * @param array $config */ - public function runScript(array $data, array $config, $tokenId = '', $errorHandling = []) + public function runScript(array $data, array $config, $tokenId = '') { if (!$this->scriptExecutor) { throw new ScriptLanguageNotSupported($this->language); } - $this->errorHandling = $errorHandling; $runner = new ScriptRunner($this->scriptExecutor); $runner->setTokenId($tokenId); $user = User::find($this->run_as_user_id); @@ -156,56 +137,7 @@ public function runScript(array $data, array $config, $tokenId = '', $errorHandl throw new \RuntimeException('A user is required to run scripts'); } - try { - return $runner->run($this->code, $data, $config, $this->timeout(), $user); - } catch (ScriptException | \RuntimeException $e) { - if ($this->retryAttempts() && $this->retryWaitTime() !== null) { - Log::info('Retry the runScript process. Attempt ' . $this->attemptedRetries . ' of ' . $this->retryAttempts()); - if ($this->attemptedRetries > $this->retryAttempts()) { - $message = __('Script failed after :attempts total attempts', ['attempts' => $this->attemptedRetries]); - $message = $message . "\n" . $e->getMessage(); - Log::error($message); - - $this->sendExecutionErrorNotification($message, $tokenId, $errorHandling); - - if ($e instanceof ScriptTimeoutException) { - throw new ScriptTimeoutException($message); - } - throw new ScriptException($message); - } - - $closure = function () use ($data, $config, $tokenId, $errorHandling) { - Log::info('Re-running the script'); - $this->runScript($data, $config, $tokenId, $errorHandling);//todo: it is important to return the correct data. - Log::info('The script completed successfully'); - }; - - $retryWaitTime = $this->retryWaitTime(); - Log::info("Waiting {$retryWaitTime} seconds before retrying."); - $this->attemptedRetries++; - - Queue::later($retryWaitTime, new ClosureJob($closure)); - - return ['output'];//todo: it is important to return the correct data. - } else { - $this->sendExecutionErrorNotification($e->getMessage(), $tokenId, $errorHandling); - throw $e; - } - } - } - - /** - * Send execution error notification. - */ - public function sendExecutionErrorNotification(string $message, string $tokenId, array $errorHandling) - { - $processRequestToken = ProcessRequestToken::find($tokenId); - if ($processRequestToken) { - $user = $processRequestToken->processRequest->processVersion->manager; - if ($user !== null) { - Notification::send($user, new ErrorExecutionNotification($processRequestToken, $message, $errorHandling)); - } - } + return $runner->run($this->code, $data, $config, $this->timeout, $user); } /** @@ -379,48 +311,4 @@ private function setDefaultExecutor() $this->language = $this->scriptExecutor->language; } } - - /** - * This method retrieves the error number by type. - * In case of not finding it, -1 is returned. - */ - private function getErrorHandlingNumberByType($type) - { - $result = Arr::get($this->errorHandling, $type, null); - if (is_numeric($result)) { - return (int) $result; - } - - return -1; - } - - /** - * Get the value of the timeout property if there hasn't been a defined error in the errorHandling array. - */ - private function timeout() - { - $result = $this->getErrorHandlingNumberByType('timeout'); - - return $result === -1 ? $this->timeout : $result; - } - - /** - * Get the value of the retryAttempts property if there hasn't been a defined error in the errorHandling array. - */ - private function retryAttempts() - { - $result = $this->getErrorHandlingNumberByType('retry_attempts'); - - return $result === -1 ? $this->retry_attempts : $result; - } - - /** - * Get the value of the retryWaitTime property if there hasn't been a defined error in the errorHandling array. - */ - private function retryWaitTime() - { - $result = $this->getErrorHandlingNumberByType('retry_wait_time'); - - return $result === -1 ? $this->retry_wait_time : $result; - } } diff --git a/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php b/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php index 201061bc1e..78ab5edf0c 100644 --- a/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php +++ b/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php @@ -83,7 +83,7 @@ private function runContainer($image, $command, $parameters, $bindings, $timeout . implode("\n", $output) ); } - Log::error('Script threw return code ' . $returnCode . 'Message: ' . implode("\n", $output)); + Log::error('Script threw return code ' . $returnCode . ' Message: ');// . implode("\n", $output)); $message = implode("\n", $output); $message .= "\n\nProcessMaker Stack:\n"; diff --git a/ProcessMaker/Traits/MakeHttpRequests.php b/ProcessMaker/Traits/MakeHttpRequests.php index 4427853f10..1eeaae5e47 100644 --- a/ProcessMaker/Traits/MakeHttpRequests.php +++ b/ProcessMaker/Traits/MakeHttpRequests.php @@ -15,6 +15,7 @@ use Mustache_Engine; use ProcessMaker\Exception\HttpInvalidArgumentException; use ProcessMaker\Exception\HttpResponseException; +use ProcessMaker\Exception\RetryableException; use ProcessMaker\Helpers\StringHelper; use ProcessMaker\Models\FormalExpression; use ProcessMaker\Models\ProcessRequest; @@ -83,10 +84,13 @@ public function request(array $data = [], array $config = []) return $this->responseWithHeaderData($this->call(...$request), $data, $config); } catch (TransferException $exception) { - $result = $this->handleRetries($data, $config); - if ($result) { - // Retry was successful - return $result; + if ($this->retryAttempts && $this->attemptedRetries !== null) { + if ($this->attemptedRetries > $this->retryAttempts) { + $this->retryMessage = __('Failed after :num total attempts', ['num' => $this->attemptedRetries + 1]); + } else { + Log::info('Retry the request. Attempt ' . $this->attemptedRetries . ' of ' . $this->retryAttempts); + throw new RetryableException($this->retryWaitTime); + } } $message = $exception->getMessage(); @@ -103,31 +107,6 @@ public function request(array $data = [], array $config = []) } } - /** - * Handle retry attempts if configured - */ - private function handleRetries(array $data, array $config) - { - if ($this->retryAttempts === 0) { - return false; - } - - if ($this->attemptedRetries >= $this->retryAttempts) { - $this->retryMessage = __('Failed after :num total attempts', ['num' => $this->attemptedRetries + 1]); - - return false; - } - - $this->attemptedRetries++; - - if ($this->retryWaitTime > 0) { - sleep($this->retryWaitTime); - } - Log::info('Retry the runScript process. Attempt ' . ($this->attemptedRetries) . ' of ' . $this->retryAttempts); - - return $this->request($data, $config); - } - private function sendErrorNotification(array $data, string $message) { if ($this->tokenId) { @@ -189,6 +168,7 @@ private function setErrorHandlingProperties(array $endpoint, array $config) 'timeout' => 'timeout', 'retryAttempts' => 'retry_attempts', 'retryWaitTime' => 'retry_wait_time', + 'attemptedRetries' => 'attempts', ]; foreach ($properties as $classProperty => $settingProperty) { diff --git a/resources/lang/en.json b/resources/lang/en.json index 229ec60929..e3d750e532 100644 --- a/resources/lang/en.json +++ b/resources/lang/en.json @@ -1695,8 +1695,8 @@ "Set maximum run retry wait time in seconds. Leave empty to use data connector default. Set to 0 for no retry wait time.": "Set maximum run retry wait time in seconds. Leave empty to use data connector default. Set to 0 for no retry wait time.", "Script failed after :attempts total attempts": "Script failed after :attempts total attempts", "Failed after :num total attempts": "Failed after :num total attempts", - "Seconds to wait before retrying. Leave empty to use script default. Set to 0 for no retry wait time.": "Seconds to wait before retrying. Leave empty to use script default. Set to 0 for no retry wait time.", - "Number of times to retry. Leave empty to use script default. Set to 0 for no retry attempts.": "Number of times to retry. Leave empty to use script default. Set to 0 for no retry attempts.", + "Seconds to wait before retrying. Leave empty to use script default. Set to 0 for no retry wait time. This setting is only used when running a script task in a process.": "Seconds to wait before retrying. Leave empty to use script default. Set to 0 for no retry wait time. This setting is only used when running a script task in a process.", + "Number of times to retry. Leave empty to use script default. Set to 0 for no retry attempts. This setting is only used when running a script task in a process.": "Number of times to retry. Leave empty to use script default. Set to 0 for no retry attempts. This setting is only used when running a script task in a process.", "View Request": "View Request", "Execution Error": "Execution Error" } diff --git a/resources/views/processes/scripts/edit.blade.php b/resources/views/processes/scripts/edit.blade.php index ea1fa7ed9b..199ae3eaa6 100644 --- a/resources/views/processes/scripts/edit.blade.php +++ b/resources/views/processes/scripts/edit.blade.php @@ -85,7 +85,7 @@ Date: Fri, 30 Jun 2023 19:42:18 -0400 Subject: [PATCH 03/12] FOUR-8993 new improvements --- ProcessMaker/Exception/RetryableException.php | 17 --- ProcessMaker/Jobs/BpmnAction.php | 98 ++++++++++++++- ProcessMaker/Jobs/RunScriptTask.php | 77 ++++++------ ProcessMaker/Jobs/RunServiceTask.php | 28 ++++- ProcessMaker/Models/ProcessRequest.php | 10 +- ProcessMaker/Models/Script.php | 6 +- .../Models/ScriptDockerBindingFilesTrait.php | 2 +- .../Nayra/Managers/WorkflowManagerDefault.php | 4 +- .../ErrorExecutionNotification.php | 1 - ProcessMaker/Traits/MakeHttpRequests.php | 117 +----------------- 10 files changed, 171 insertions(+), 189 deletions(-) delete mode 100644 ProcessMaker/Exception/RetryableException.php diff --git a/ProcessMaker/Exception/RetryableException.php b/ProcessMaker/Exception/RetryableException.php deleted file mode 100644 index cf179a6f8c..0000000000 --- a/ProcessMaker/Exception/RetryableException.php +++ /dev/null @@ -1,17 +0,0 @@ -engine->runToNextState(); - } catch (RetryableException $e) { - Log::info('Re-Dispatching. Attempts: ' . $this->attempts() . ', Wait time: ' . $e->retry_wait_time); - $this->release($e->retry_wait_time); } catch (Throwable $exception) { - Log::error('%% ' . $exception->getMessage()); + Log::error($exception->getMessage()); // Change the Request to error status $request = !$this->instance && $this instanceof StartEvent ? $response : $this->instance; if ($request) { @@ -84,7 +95,7 @@ public function handle() * * @return array */ - protected function loadContext() + private function loadContext() { //Load the process definition if (isset($this->instanceId)) { @@ -298,4 +309,79 @@ public function __destruct() $this->lock = null; gc_collect_cycles(); } + + /** + * This sets the property errorHandling, and if the parameter doesn't exist, + * it returns the current value of property errorHandling. + * + * @param mixed $value + * @return mixed + */ + public function errorHandling($value = null) + { + if (is_array($value) && !empty($value)) { + $array = [ + 'timeout', + 'retry_attempts', + 'retry_wait_time' + ]; + foreach ($array as $val) { + $digit = 0; + if (is_string($value[$val]) && ctype_digit($value[$val])) { + $digit = intval($value[$val]); + } + if (is_int($value[$val])) { + $digit = $value[$val]; + } + $value[$val] = $digit; + } + $this->errorHandling = $value; + } + return $this->errorHandling; + } + + /** + * This retrieves the value of errorHandling['timeout']. + * + * @return int + */ + public function timeout() + { + return $this->errorHandling['timeout']; + } + + /** + * This retrieves the value of errorHandling['retry_attempts']. + * + * @return int + */ + public function retryAttempts() + { + return $this->errorHandling['retry_attempts']; + } + + /** + * This retrieves the value of errorHandling['retry_wait_time']. + * + * @return int + */ + public function retryWaitTime() + { + return $this->errorHandling['retry_wait_time']; + } + + /** + * Send execution error notification. + */ + public function sendExecutionErrorNotification(string $message, string $tokenId, array $errorHandling) + { + $processRequestToken = ProcessRequestToken::find($tokenId); + if ($processRequestToken) { + $user = $processRequestToken->processRequest->processVersion->manager; + if ($user !== null) { + Log::info('Send Execution Error Notification: ' . $message); + Notification::send($user, new ErrorExecutionNotification($processRequestToken, $message, $errorHandling)); + } + } + } } diff --git a/ProcessMaker/Jobs/RunScriptTask.php b/ProcessMaker/Jobs/RunScriptTask.php index 42c638990a..26278207e0 100644 --- a/ProcessMaker/Jobs/RunScriptTask.php +++ b/ProcessMaker/Jobs/RunScriptTask.php @@ -4,9 +4,7 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Support\Facades\Log; -use ProcessMaker\Exception\RetryableException; use ProcessMaker\Exception\ScriptException; -use Illuminate\Support\Facades\Notification; use ProcessMaker\Facades\WorkflowManager; use ProcessMaker\Managers\DataManager; use ProcessMaker\Models\Process as Definitions; @@ -15,7 +13,6 @@ use ProcessMaker\Models\Script; use ProcessMaker\Models\ScriptExecutor; use ProcessMaker\Nayra\Contracts\Bpmn\ScriptTaskInterface; -use ProcessMaker\Notifications\ErrorExecutionNotification; use Throwable; class RunScriptTask extends BpmnAction implements ShouldQueue @@ -28,10 +25,6 @@ class RunScriptTask extends BpmnAction implements ShouldQueue public $data; - public $tries; - - //public $backoff = 60; - /** * Create a new job instance. * @@ -42,15 +35,12 @@ class RunScriptTask extends BpmnAction implements ShouldQueue */ public function __construct(Definitions $definitions, ProcessRequest $instance, ProcessRequestToken $token, array $data) { - Log::error('$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$'); $this->onQueue('bpmn'); $this->definitionsId = $definitions->getKey(); $this->instanceId = $instance->getKey(); $this->tokenId = $token->getKey(); $this->elementId = $token->getProperty('element_ref'); $this->data = $data; - - $this->tries = 4;// } /** @@ -67,9 +57,6 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e $scriptRef = $element->getProperty('scriptRef'); $configuration = json_decode($element->getProperty('config'), true); $errorHandling = json_decode($element->getProperty('errorHandling'), true) ?? []; - $errorHandling['attempts'] = $this->attempts(); - $errorHandling['retry_attempts'] = ctype_digit($errorHandling['retry_attempts']) ? intval($errorHandling['retry_attempts']) : 0; - $errorHandling['retry_wait_time'] = ctype_digit($errorHandling['retry_wait_time']) ? intval($errorHandling['retry_wait_time']) : 0; // Check to see if we've failed parsing. If so, let's convert to empty array. if ($configuration === null) { @@ -92,10 +79,25 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e $script = Script::findOrFail($scriptRef)->versionFor($instance); } + /** + * If the configuration values for the 'ScriptTask' element do not exist, + * then the values from the 'Script' are attempted to be taken. + * If these values do not exist either, the fallback values are the + * globally scoped values (config/horizon.php). + */ + if (!is_array($errorHandling) || empty($errorHandling)) { + $errorHandling = [ + 'timeout' => $script->timeout, + 'retry_attempts' => $script->retry_attempts, + 'retry_wait_time' => $script->retry_wait_time + ]; + } + $this->errorHandling($errorHandling); + $this->unlock(); $dataManager = new DataManager(); $data = $dataManager->getData($token); - $response = $script->runScript($data, $configuration, $token->getId()); + $response = $script->runScript($data, $configuration, $token->getId(), $this->timeout()); $this->withUpdatedContext(function ($engine, $instance, $element, $processModel, $token) use ($response) { // Exit if the task was completed or closed @@ -123,16 +125,17 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e $token->setProperty('error', $error); $token->logError($exception, $element); - Log::error('Script failed: ' . $scriptRef . ' - ');// . $exception->getMessage()); - //Log::error($exception->getTraceAsString()); - - if ($errorHandling['retry_attempts'] > 0) { - if ($this->attempts() <= $errorHandling['retry_attempts']) { - Log::info('Retry the runScript process. Attempt ' . $this->attempts() . ' of ' . $errorHandling['retry_attempts']); - throw new RetryableException($errorHandling['retry_wait_time']); + Log::error('Script failed: ' . $scriptRef . ' - ' . $exception->getMessage()); + Log::error($exception->getTraceAsString()); + + if ($this->retryAttempts() > 0) { + if ($this->attempts() <= $this->retryAttempts()) { + Log::info('Retry the runScript process. Attempt ' . $this->attempts() . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); + $this->release($this->retryWaitTime()); + return; } - - $message = __('Script failed after :attempts total attempts', ['attempts' => $this->attempts()]); + + $message = __('Script failed after :attempts total attempts', ['attempts' => $this->attempts() - 1]); $message = $message . "\n" . $exception->getMessage(); $this->sendExecutionErrorNotification($message, $token->getId(), $errorHandling); @@ -147,20 +150,6 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e } } - /** - * Send execution error notification. - */ - public function sendExecutionErrorNotification(string $message, string $tokenId, array $errorHandling) - { - $processRequestToken = ProcessRequestToken::find($tokenId); - if ($processRequestToken) { - $user = $processRequestToken->processRequest->processVersion->manager; - if ($user !== null) { - Notification::send($user, new ErrorExecutionNotification($processRequestToken, $message, $errorHandling)); - } - } - } - /** * When Job fails */ @@ -171,14 +160,22 @@ public function failed(Throwable $exception) return; } + if (get_class($exception) === "Illuminate\\Queue\\MaxAttemptsExceededException") { + $message = 'This is a type MaxAttemptsExceededException exception, it appears ' + . 'that the global value configured in config/horizon.php has been exceeded ' + . 'in the retries. Please consult with your main administrator.'; + Log::error($message); + } Log::error('Script (#' . $this->tokenId . ') failed: ' . $exception->getMessage()); $token = ProcessRequestToken::find($this->tokenId); if ($token) { $element = $token->getBpmnDefinition(); $token->setStatus(ScriptTaskInterface::TOKEN_STATE_FAILING); - $error = $element->getRepository()->createError(); - $error->setName($exception->getMessage()); - $token->setProperty('error', $error); + if (method_exists($element, 'getRepository')) { + $error = $element->getRepository()->createError(); + $error->setName($exception->getMessage()); + $token->setProperty('error', $error); + } Log::error($exception->getTraceAsString()); $token->save(); } diff --git a/ProcessMaker/Jobs/RunServiceTask.php b/ProcessMaker/Jobs/RunServiceTask.php index c01c441d51..ccc0c2e393 100644 --- a/ProcessMaker/Jobs/RunServiceTask.php +++ b/ProcessMaker/Jobs/RunServiceTask.php @@ -66,7 +66,6 @@ public function action(ProcessRequestToken $token = null, ServiceTaskInterface $ $implementation = $element->getImplementation(); $configuration = json_decode($element->getProperty('config'), true); $errorHandling = json_decode($element->getProperty('errorHandling'), true) ?? []; - $errorHandling['attempts'] = $this->attempts(); // Check to see if we've failed parsing. If so, let's convert to empty array. if ($configuration === null) { @@ -85,13 +84,16 @@ public function action(ProcessRequestToken $token = null, ServiceTaskInterface $ throw new ScriptException('Service task not implemented: ' . $implementation); } + //todo: It is necessary to obtain the configuration values of the "dataSource" for the default values. + $this->errorHandling($errorHandling); + $this->unlock(); $dataManager = new DataManager(); $data = $dataManager->getData($token); if ($existsImpl) { $response = [ - 'output' => WorkflowManager::runServiceImplementation($implementation, $data, $configuration, $token->getId(), $errorHandling), + 'output' => WorkflowManager::runServiceImplementation($implementation, $data, $configuration, $token->getId()), ]; } else { $response = $script->runScript($data, $configuration, $token->getId(), $errorHandling); @@ -121,6 +123,22 @@ public function action(ProcessRequestToken $token = null, ServiceTaskInterface $ $token->setProperty('error', $error); Log::info('Service task failed: ' . $implementation . ' - ' . $exception->getMessage()); Log::error($exception->getTraceAsString()); + + if ($this->retryAttempts() > 0) { + if ($this->attempts() <= $this->retryAttempts()) { + Log::info('Retry the runScript process. Attempt ' . $this->attempts() . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); + $this->release($this->retryWaitTime()); + return; + } + $message = __('Failed after :num total attempts', ['num' => $this->attempts()]); + $message = $message . "\n" . $exception->getMessage(); + + $this->sendExecutionErrorNotification($message, $token->getId(), $errorHandling); + throw $exception; + } else { + $this->sendExecutionErrorNotification($exception->getMessage(), $token->getId(), $errorHandling); + throw $exception; + } } } @@ -134,6 +152,12 @@ public function failed(Throwable $exception) return; } + if (get_class($exception) === "Illuminate\\Queue\\MaxAttemptsExceededException") { + $message = 'This is a type MaxAttemptsExceededException exception, it appears ' + . 'that the global value configured in config/horizon.php has been exceeded ' + . 'in the retries. Please consult with your main administrator.'; + Log::error($message); + } Log::error('Script (#' . $this->tokenId . ') failed: ' . $exception->getMessage()); Log::error($exception->getTraceAsString()); $token = ProcessRequestToken::find($this->tokenId); diff --git a/ProcessMaker/Models/ProcessRequest.php b/ProcessMaker/Models/ProcessRequest.php index fe903c5890..03f8643a57 100644 --- a/ProcessMaker/Models/ProcessRequest.php +++ b/ProcessMaker/Models/ProcessRequest.php @@ -549,12 +549,10 @@ public function logError(Throwable $exception, FlowElementInterface $element = n $errors[] = $error; $this->errors = $errors; $this->status = 'ERROR'; - // $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 5); - // \Log::info("TRACE", $trace); - // \Log::error($exception); - // if (!$this->isNonPersistent()) { - // $this->save(); - // } + \Log::error($exception); + if (!$this->isNonPersistent()) { + $this->save(); + } } public function childRequests() diff --git a/ProcessMaker/Models/Script.php b/ProcessMaker/Models/Script.php index fbdb933c51..3ca23655ab 100644 --- a/ProcessMaker/Models/Script.php +++ b/ProcessMaker/Models/Script.php @@ -76,6 +76,8 @@ class Script extends ProcessMakerModel implements ScriptInterface protected $casts = [ 'timeout' => 'integer', + 'retry_attempts' => 'integer', + 'retry_wait_time' => 'integer' ]; /** @@ -125,7 +127,7 @@ public static function rules($existing = null) * @param array $data * @param array $config */ - public function runScript(array $data, array $config, $tokenId = '') + public function runScript(array $data, array $config, $tokenId = '', $timeout = 60) { if (!$this->scriptExecutor) { throw new ScriptLanguageNotSupported($this->language); @@ -137,7 +139,7 @@ public function runScript(array $data, array $config, $tokenId = '') throw new \RuntimeException('A user is required to run scripts'); } - return $runner->run($this->code, $data, $config, $this->timeout, $user); + return $runner->run($this->code, $data, $config, $timeout, $user); } /** diff --git a/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php b/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php index 78ab5edf0c..201061bc1e 100644 --- a/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php +++ b/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php @@ -83,7 +83,7 @@ private function runContainer($image, $command, $parameters, $bindings, $timeout . implode("\n", $output) ); } - Log::error('Script threw return code ' . $returnCode . ' Message: ');// . implode("\n", $output)); + Log::error('Script threw return code ' . $returnCode . 'Message: ' . implode("\n", $output)); $message = implode("\n", $output); $message .= "\n\nProcessMaker Stack:\n"; diff --git a/ProcessMaker/Nayra/Managers/WorkflowManagerDefault.php b/ProcessMaker/Nayra/Managers/WorkflowManagerDefault.php index e82a230fd3..3ef036412b 100644 --- a/ProcessMaker/Nayra/Managers/WorkflowManagerDefault.php +++ b/ProcessMaker/Nayra/Managers/WorkflowManagerDefault.php @@ -357,11 +357,11 @@ class_exists($this->serviceTaskImplementations[$implementation]); * * @return mixed */ - public function runServiceImplementation($implementation, array $data, array $config, $tokenId = '', $errorHandling = []) + public function runServiceImplementation($implementation, array $data, array $config, $tokenId = '') { $class = $this->serviceTaskImplementations[$implementation]; $service = new $class(); - return $service->run($data, $config, $tokenId, $errorHandling); + return $service->run($data, $config, $tokenId); } } diff --git a/ProcessMaker/Notifications/ErrorExecutionNotification.php b/ProcessMaker/Notifications/ErrorExecutionNotification.php index d5873299a3..60775b5718 100644 --- a/ProcessMaker/Notifications/ErrorExecutionNotification.php +++ b/ProcessMaker/Notifications/ErrorExecutionNotification.php @@ -3,7 +3,6 @@ namespace ProcessMaker\Notifications; use Illuminate\Bus\Queueable; -use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Notifications\Messages\BroadcastMessage; use Illuminate\Notifications\Messages\MailMessage; use Illuminate\Notifications\Notification; diff --git a/ProcessMaker/Traits/MakeHttpRequests.php b/ProcessMaker/Traits/MakeHttpRequests.php index 1eeaae5e47..129fa8ace8 100644 --- a/ProcessMaker/Traits/MakeHttpRequests.php +++ b/ProcessMaker/Traits/MakeHttpRequests.php @@ -6,21 +6,15 @@ use GuzzleHttp\Client; use GuzzleHttp\Exception\ClientException; use GuzzleHttp\Exception\GuzzleException; -use GuzzleHttp\Exception\TransferException; use GuzzleHttp\Psr7\Request; use GuzzleHttp\Psr7\Response; use Illuminate\Support\Arr; use Illuminate\Support\Facades\Log; -use Illuminate\Support\Facades\Notification; use Mustache_Engine; use ProcessMaker\Exception\HttpInvalidArgumentException; use ProcessMaker\Exception\HttpResponseException; -use ProcessMaker\Exception\RetryableException; use ProcessMaker\Helpers\StringHelper; use ProcessMaker\Models\FormalExpression; -use ProcessMaker\Models\ProcessRequest; -use ProcessMaker\Models\ProcessRequestToken; -use ProcessMaker\Notifications\ErrorExecutionNotification; use Psr\Http\Message\ResponseInterface; trait MakeHttpRequests @@ -40,22 +34,6 @@ trait MakeHttpRequests private $mustache = null; - private $timeout = 0; - - private $retryAttempts = 0; - - private $retryWaitTime = 0; - - private $attemptedRetries = 0; - - private $retryMessage = null; - - private $inappNotification = false; - - private $emailNotification = false; - - private $tokenId = null; - private function getMustache() { if ($this->mustache === null) { @@ -83,42 +61,8 @@ public function request(array $data = [], array $config = []) $request = $this->prepareRequestWithOutboundConfig($data, $config); return $this->responseWithHeaderData($this->call(...$request), $data, $config); - } catch (TransferException $exception) { - if ($this->retryAttempts && $this->attemptedRetries !== null) { - if ($this->attemptedRetries > $this->retryAttempts) { - $this->retryMessage = __('Failed after :num total attempts', ['num' => $this->attemptedRetries + 1]); - } else { - Log::info('Retry the request. Attempt ' . $this->attemptedRetries . ' of ' . $this->retryAttempts); - throw new RetryableException($this->retryWaitTime); - } - } - - $message = $exception->getMessage(); - $message = $this->retryMessage ? $this->retryMessage . "\n" . $message : $message; - - $this->sendErrorNotification($data, $message); - - if ($exception instanceof ClientException) { - $response = $exception->getResponse(); - throw new HttpResponseException($response, $message); - } else { - throw new \Exception($message); - } - } - } - - private function sendErrorNotification(array $data, string $message) - { - if ($this->tokenId) { - $processRequestToken = ProcessRequestToken::findOrFail($this->tokenId); - $user = $processRequestToken->processRequest->processVersion->manager; - if ($user !== null) { - $errorHandling = [ - 'inapp_notification' => $this->inappNotification, - 'email_notification' => $this->emailNotification, - ]; - Notification::send($user, new ErrorExecutionNotification($processRequestToken, $message, $errorHandling)); - } + } catch (ClientException $exception) { + throw new HttpResponseException($exception->getResponse()); } } @@ -154,54 +98,6 @@ private function prepareRequest(array &$data, array &$config) return $request; } - /** - * Set error handling properties from endpoint config or from BPMN errorHandling - * - * @param array $endpoint - * @param array $config - * - * @return void - */ - private function setErrorHandlingProperties(array $endpoint, array $config) - { - $properties = [ - 'timeout' => 'timeout', - 'retryAttempts' => 'retry_attempts', - 'retryWaitTime' => 'retry_wait_time', - 'attemptedRetries' => 'attempts', - ]; - - foreach ($properties as $classProperty => $settingProperty) { - $this->$classProperty = (int) Arr::get($endpoint, $settingProperty, 0); - $bpmnSetting = Arr::get($config, "errorHandling.{$settingProperty}", null); - if (is_numeric($bpmnSetting)) { - $this->$classProperty = (int) $bpmnSetting; - } - } - - $this->inappNotification = Arr::get($endpoint, 'inapp_notification', false); - $this->emailNotification = Arr::get($endpoint, 'email_notification', false); - - $this->tokenId = Arr::get($config, 'tokenId', null); - - if (Arr::get($config, 'errorHandling.inapp_notification', false) === true) { - $this->inappNotification = true; - } - if (Arr::get($config, 'errorHandling.email_notification', false) === true) { - $this->emailNotification = true; - } - } - - /** - * Return the value of $this->timeout - * - * @return int - */ - public function getTimeout() - { - return $this->timeout; - } - /** * Prepare data to be used in body (mustache) * @@ -305,8 +201,7 @@ private function prepareRequestWithOutboundConfig(array $requestData, array &$co $request = [$method, $url, $headers, $body, $bodyType]; $request = $this->addAuthorizationHeaders(...$request); - - $this->setErrorHandlingProperties($endpoint, $config); + //todo: The values of $endpoint must be passed somehow to RunServiceTask inside action method. return $request; } @@ -535,11 +430,9 @@ private function call($method, $url, array $headers, $body, $bodyType) { $client = $this->client ?? app()->make(Client::class, [ 'config' => [ - 'verify' => $this->verifySsl, - 'timeout' => $this->getTimeout(), - ], + 'verify' => $this->verifySsl + ] ]); - $options = []; if ($bodyType === 'form-data') { $options['form_params'] = json_decode($body, true); From f752f40fc75307e9b76ea05c0ac22714b78a5bf1 Mon Sep 17 00:00:00 2001 From: Nolan Ehrstrom Date: Mon, 3 Jul 2023 13:23:36 -0700 Subject: [PATCH 04/12] Move error handling logic to its own class --- ProcessMaker/Jobs/BpmnAction.php | 87 ----------------- ProcessMaker/Jobs/ErrorHandling.php | 140 ++++++++++++++++++++++++++++ ProcessMaker/Jobs/RunScriptTask.php | 50 ++-------- tests/Jobs/ErrorHandlingTest.php | 100 ++++++++++++++++++++ 4 files changed, 250 insertions(+), 127 deletions(-) create mode 100644 ProcessMaker/Jobs/ErrorHandling.php create mode 100644 tests/Jobs/ErrorHandlingTest.php diff --git a/ProcessMaker/Jobs/BpmnAction.php b/ProcessMaker/Jobs/BpmnAction.php index 8a544deddd..52a68439d7 100644 --- a/ProcessMaker/Jobs/BpmnAction.php +++ b/ProcessMaker/Jobs/BpmnAction.php @@ -46,18 +46,6 @@ abstract class BpmnAction implements ShouldQueue */ private $lock; - /** - * This sets the set of values: 'timeout', 'retry_attempts', 'retry_wait_time' - * that allow managing the retry of the job if it has failed. - * To read the values, use the: timeout(), retryAttempts(), retryWaitTime() methods. - * To set this value, use the method `errorHandling()` that accepts an array. If the - * parameter is not defined, the method returns the current value of this property. - * This property can be both set and get using the method errorHandling(). - * - * @var array - */ - private $errorHandling = null; - /** * Execute the job. * @@ -309,79 +297,4 @@ public function __destruct() $this->lock = null; gc_collect_cycles(); } - - /** - * This sets the property errorHandling, and if the parameter doesn't exist, - * it returns the current value of property errorHandling. - * - * @param mixed $value - * @return mixed - */ - public function errorHandling($value = null) - { - if (is_array($value) && !empty($value)) { - $array = [ - 'timeout', - 'retry_attempts', - 'retry_wait_time' - ]; - foreach ($array as $val) { - $digit = 0; - if (is_string($value[$val]) && ctype_digit($value[$val])) { - $digit = intval($value[$val]); - } - if (is_int($value[$val])) { - $digit = $value[$val]; - } - $value[$val] = $digit; - } - $this->errorHandling = $value; - } - return $this->errorHandling; - } - - /** - * This retrieves the value of errorHandling['timeout']. - * - * @return int - */ - public function timeout() - { - return $this->errorHandling['timeout']; - } - - /** - * This retrieves the value of errorHandling['retry_attempts']. - * - * @return int - */ - public function retryAttempts() - { - return $this->errorHandling['retry_attempts']; - } - - /** - * This retrieves the value of errorHandling['retry_wait_time']. - * - * @return int - */ - public function retryWaitTime() - { - return $this->errorHandling['retry_wait_time']; - } - - /** - * Send execution error notification. - */ - public function sendExecutionErrorNotification(string $message, string $tokenId, array $errorHandling) - { - $processRequestToken = ProcessRequestToken::find($tokenId); - if ($processRequestToken) { - $user = $processRequestToken->processRequest->processVersion->manager; - if ($user !== null) { - Log::info('Send Execution Error Notification: ' . $message); - Notification::send($user, new ErrorExecutionNotification($processRequestToken, $message, $errorHandling)); - } - } - } } diff --git a/ProcessMaker/Jobs/ErrorHandling.php b/ProcessMaker/Jobs/ErrorHandling.php new file mode 100644 index 0000000000..b5468cced2 --- /dev/null +++ b/ProcessMaker/Jobs/ErrorHandling.php @@ -0,0 +1,140 @@ +getProperty('errorHandling'), true) ?? []; + \Log::info('ERROR HANDLING', ['eh' => $errorHandling]); + \Log::info('model: ' . get_class($this->model)); + + if ($this->model instanceof ScriptVersion) { + $script = $this->model; + + if (!is_array($errorHandling) || empty($errorHandling)) { + $errorHandling = [ + 'timeout' => $script->timeout, + 'retry_attempts' => $script->retry_attempts, + 'retry_wait_time' => $script->retry_wait_time, + ]; + } + $this->errorHandling($errorHandling); + } + + // TODO data sources + } + + public function handleRetries($job, $exception) + { + $message = $exception->getMessage(); + + if ($this->retryAttempts() > 0) { + if ($job->attempts() <= $this->retryAttempts()) { + Log::info('Retry the runScript process. Attempt ' . $job->attempts() . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); + $job->release($this->retryWaitTime()); + + return $message; + } + + $message = __('Script failed after :attempts total attempts', ['attempts' => $job->attempts() - 1]) . "\n" . $message; + + $this->sendExecutionErrorNotification($message); + + return $message; + } else { + $this->sendExecutionErrorNotification($message); + } + + return $message; + } + + /** + * Send execution error notification. + */ + public function sendExecutionErrorNotification(string $message) + { + if ($this->processRequestToken) { + $user = $this->processRequestToken->processRequest->processVersion->manager; + if ($user !== null) { + Log::info('Send Execution Error Notification: ' . $message); + Notification::send($user, new ErrorExecutionNotification($this->processRequestToken, $message, $this->bpmnSetting)); + } + } + } + + /** + * This sets the property errorHandling, and if the parameter doesn't exist, + * it returns the current value of property errorHandling. + * + * @param mixed $value + * @return mixed + */ + public function errorHandling($value = null) + { + \Log::info('errorHandling', ['value' => $value]); + if (is_array($value) && !empty($value)) { + $array = [ + 'timeout', + 'retry_attempts', + 'retry_wait_time', + ]; + foreach ($array as $val) { + $digit = 0; + if (is_string($value[$val]) && ctype_digit($value[$val])) { + $digit = intval($value[$val]); + } + if (is_int($value[$val])) { + $digit = $value[$val]; + } + $value[$val] = $digit; + } + $this->errorHandling = $value; + } + + return $this->errorHandling; + } + + /** + * This retrieves the value of errorHandling['timeout']. + * + * @return int + */ + public function timeout() + { + return $this->errorHandling['timeout']; + } + + /** + * This retrieves the value of errorHandling['retry_attempts']. + * + * @return int + */ + public function retryAttempts() + { + return $this->errorHandling['retry_attempts']; + } + + /** + * This retrieves the value of errorHandling['retry_wait_time']. + * + * @return int + */ + public function retryWaitTime() + { + return $this->errorHandling['retry_wait_time']; + } +} diff --git a/ProcessMaker/Jobs/RunScriptTask.php b/ProcessMaker/Jobs/RunScriptTask.php index 26278207e0..86d3555e66 100644 --- a/ProcessMaker/Jobs/RunScriptTask.php +++ b/ProcessMaker/Jobs/RunScriptTask.php @@ -56,7 +56,6 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e } $scriptRef = $element->getProperty('scriptRef'); $configuration = json_decode($element->getProperty('config'), true); - $errorHandling = json_decode($element->getProperty('errorHandling'), true) ?? []; // Check to see if we've failed parsing. If so, let's convert to empty array. if ($configuration === null) { @@ -79,25 +78,12 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e $script = Script::findOrFail($scriptRef)->versionFor($instance); } - /** - * If the configuration values for the 'ScriptTask' element do not exist, - * then the values from the 'Script' are attempted to be taken. - * If these values do not exist either, the fallback values are the - * globally scoped values (config/horizon.php). - */ - if (!is_array($errorHandling) || empty($errorHandling)) { - $errorHandling = [ - 'timeout' => $script->timeout, - 'retry_attempts' => $script->retry_attempts, - 'retry_wait_time' => $script->retry_wait_time - ]; - } - $this->errorHandling($errorHandling); + $errorHandling = new ErrorHandling($element, $script, $token); $this->unlock(); $dataManager = new DataManager(); $data = $dataManager->getData($token); - $response = $script->runScript($data, $configuration, $token->getId(), $this->timeout()); + $response = $script->runScript($data, $configuration, $token->getId(), $errorHandling->timeout()); $this->withUpdatedContext(function ($engine, $instance, $element, $processModel, $token) use ($response) { // Exit if the task was completed or closed @@ -119,34 +105,18 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e } catch (Throwable $exception) { $token->setStatus(ScriptTaskInterface::TOKEN_STATE_FAILING); + $message = $errorHandling->handleRetries($this, $exception); + $error = $element->getRepository()->createError(); - $error->setName($exception->getMessage()); + $error->setName($message); $token->setProperty('error', $error); - $token->logError($exception, $element); + $exceptionClass = get_class($exception); + $modifiedException = new $exceptionClass($message); + $token->logError($modifiedException, $element); - Log::error('Script failed: ' . $scriptRef . ' - ' . $exception->getMessage()); + Log::error('Script failed: ' . $scriptRef . ' - ' . $message); Log::error($exception->getTraceAsString()); - - if ($this->retryAttempts() > 0) { - if ($this->attempts() <= $this->retryAttempts()) { - Log::info('Retry the runScript process. Attempt ' . $this->attempts() . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); - $this->release($this->retryWaitTime()); - return; - } - - $message = __('Script failed after :attempts total attempts', ['attempts' => $this->attempts() - 1]); - $message = $message . "\n" . $exception->getMessage(); - - $this->sendExecutionErrorNotification($message, $token->getId(), $errorHandling); - if ($exception instanceof ScriptTimeoutException) { - throw new ScriptTimeoutException($message); - } - throw new ScriptException($message); - } else { - $this->sendExecutionErrorNotification($exception->getMessage(), $token->getId(), $errorHandling); - throw $exception; - } } } @@ -160,7 +130,7 @@ public function failed(Throwable $exception) return; } - if (get_class($exception) === "Illuminate\\Queue\\MaxAttemptsExceededException") { + if (get_class($exception) === 'Illuminate\\Queue\\MaxAttemptsExceededException') { $message = 'This is a type MaxAttemptsExceededException exception, it appears ' . 'that the global value configured in config/horizon.php has been exceeded ' . 'in the retries. Please consult with your main administrator.'; diff --git a/tests/Jobs/ErrorHandlingTest.php b/tests/Jobs/ErrorHandlingTest.php new file mode 100644 index 0000000000..61bb918581 --- /dev/null +++ b/tests/Jobs/ErrorHandlingTest.php @@ -0,0 +1,100 @@ + $bpmnTimeout, 'retry_attempts' => $bpmnRetryAttempts, 'retry_wait_time' => $bpmnRetryWaitTime]; + $element = Mockery::mock(); + $element->shouldReceive('getProperty')->andReturn(json_encode($errorHandling)); + $job = Mockery::mock(RunScriptTask::class); + $job->shouldReceive('attempts')->andReturn($attempt); + $job->shouldReceive('release')->with($expectedWaitTime)->times($expectedReleaseCount); + + $script = Script::factory()->create(['timeout' => $modelTimeout, 'retry_attempts' => $modelRetryAttempts, 'retry_wait_time' => $modelRetryWaitTime]); + $token = ProcessRequestToken::factory()->create(); + + $errorHandling = new ErrorHandling($element, $script, $token); + $this->assertEquals($expectedTimeout, $errorHandling->timeout()); + $errorHandling->handleRetries($job, new \RuntimeException('error')); + } + + public function testRetry() + { + $this->runAssertions([ + 'attempt' => 3, + 'expectedTimeout' => 15, + 'expectedReleaseCount' => 1, // retry the job + 'expectedWaitTime' => 5, + + 'modelTimeout' => 8, + 'modelRetryAttempts' => 2, + 'modelRetryWaitTime' => 6, + 'bpmnTimeout' => 15, + 'bpmnRetryAttempts' => 3, + 'bpmnRetryWaitTime' => 5, + ]); + } + + public function testDoNotRetry() + { + $this->runAssertions([ + 'attempt' => 4, + 'expectedTimeout' => 15, + 'expectedReleaseCount' => 0, // do not retry the job because attempt is 4 and bpmnRetryAttempts is 3 + 'expectedWaitTime' => 5, + + 'modelTimeout' => 8, + 'modelRetryAttempts' => 2, + 'modelRetryWaitTime' => 6, + 'bpmnTimeout' => 15, + 'bpmnRetryAttempts' => 3, + 'bpmnRetryWaitTime' => 5, + ]); + } + + public function testRetryWaitFromModel() + { + $this->runAssertions([ + 'attempt' => 4, + 'expectedTimeout' => 15, + 'expectedReleaseCount' => 0, + 'expectedWaitTime' => 6, // use modelRetryWaitTime because bpmnWaitTime is empty + + 'modelTimeout' => 8, + 'modelRetryAttempts' => 2, + 'modelRetryWaitTime' => 6, + 'bpmnTimeout' => 15, + 'bpmnRetryAttempts' => 3, + 'bpmnRetryWaitTime' => '', + ]); + } + + public function testTimeoutFromModel() + { + $this->runAssertions([ + 'attempt' => 3, + 'expectedTimeout' => 8, // use modelTimeout because bpmnTimeout is empty + 'expectedReleaseCount' => 1, + 'expectedWaitTime' => 5, + + 'modelTimeout' => 8, + 'modelRetryAttempts' => 2, + 'modelRetryWaitTime' => 6, + 'bpmnTimeout' => '', + 'bpmnRetryAttempts' => 3, + 'bpmnRetryWaitTime' => 5, + ]); + } +} From b0324bb132a983451ae8adc4e32a588b9f8905cd Mon Sep 17 00:00:00 2001 From: Nolan Ehrstrom Date: Mon, 3 Jul 2023 13:25:13 -0700 Subject: [PATCH 05/12] Remove debugging --- ProcessMaker/Jobs/ErrorHandling.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/ProcessMaker/Jobs/ErrorHandling.php b/ProcessMaker/Jobs/ErrorHandling.php index b5468cced2..741f5ba528 100644 --- a/ProcessMaker/Jobs/ErrorHandling.php +++ b/ProcessMaker/Jobs/ErrorHandling.php @@ -19,8 +19,6 @@ public function __construct( public $processRequestToken, ) { $errorHandling = json_decode($element->getProperty('errorHandling'), true) ?? []; - \Log::info('ERROR HANDLING', ['eh' => $errorHandling]); - \Log::info('model: ' . get_class($this->model)); if ($this->model instanceof ScriptVersion) { $script = $this->model; From 164c176a214ab899fcdec03e7d44dac532f5f5bb Mon Sep 17 00:00:00 2001 From: Nolan Ehrstrom Date: Mon, 3 Jul 2023 16:23:00 -0700 Subject: [PATCH 06/12] Create a new job instead of using the release method --- ProcessMaker/Jobs/ErrorHandling.php | 26 ++++++++++++++++++++++---- ProcessMaker/Jobs/RunScriptTask.php | 5 ++++- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/ProcessMaker/Jobs/ErrorHandling.php b/ProcessMaker/Jobs/ErrorHandling.php index 741f5ba528..6f900bdca9 100644 --- a/ProcessMaker/Jobs/ErrorHandling.php +++ b/ProcessMaker/Jobs/ErrorHandling.php @@ -5,6 +5,9 @@ use Illuminate\Support\Arr; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Notification; +use ProcessMaker\Models\Process; +use ProcessMaker\Models\ProcessRequest; +use ProcessMaker\Models\ProcessRequestToken; use ProcessMaker\Models\Script; use ProcessMaker\Models\ScriptVersion; use ProcessMaker\Notifications\ErrorExecutionNotification; @@ -41,14 +44,14 @@ public function handleRetries($job, $exception) $message = $exception->getMessage(); if ($this->retryAttempts() > 0) { - if ($job->attempts() <= $this->retryAttempts()) { - Log::info('Retry the runScript process. Attempt ' . $job->attempts() . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); - $job->release($this->retryWaitTime()); + if ($job->attemptNum <= $this->retryAttempts()) { + Log::info('Retry the runScript process. Attempt ' . $job->attemptNum . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); + $this->requeue($job); return $message; } - $message = __('Script failed after :attempts total attempts', ['attempts' => $job->attempts() - 1]) . "\n" . $message; + $message = __('Script failed after :attempts total attempts', ['attempts' => $job->attemptNum]) . "\n" . $message; $this->sendExecutionErrorNotification($message); @@ -60,6 +63,21 @@ public function handleRetries($job, $exception) return $message; } + private function requeue($job) + { + $class = get_class($job); + $newJob = new $class( + Process::findOrFail($job->definitionsId), + ProcessRequest::findOrFail($job->instanceId), + ProcessRequestToken::findOrFail($job->tokenId), + $job->data, + $job->attemptNum + 1 + ); + \Log::info('Requeue the runScript process. Attempt ' . $job->attemptNum . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); + $newJob->delay($this->retryWaitTime()); + dispatch($newJob); + } + /** * Send execution error notification. */ diff --git a/ProcessMaker/Jobs/RunScriptTask.php b/ProcessMaker/Jobs/RunScriptTask.php index 86d3555e66..abaf76b837 100644 --- a/ProcessMaker/Jobs/RunScriptTask.php +++ b/ProcessMaker/Jobs/RunScriptTask.php @@ -25,6 +25,8 @@ class RunScriptTask extends BpmnAction implements ShouldQueue public $data; + public $attemptNum; + /** * Create a new job instance. * @@ -33,7 +35,7 @@ class RunScriptTask extends BpmnAction implements ShouldQueue * @param \ProcessMaker\Models\ProcessRequestToken $token * @param array $data */ - public function __construct(Definitions $definitions, ProcessRequest $instance, ProcessRequestToken $token, array $data) + public function __construct(Definitions $definitions, ProcessRequest $instance, ProcessRequestToken $token, array $data, $attemptNum = 1) { $this->onQueue('bpmn'); $this->definitionsId = $definitions->getKey(); @@ -41,6 +43,7 @@ public function __construct(Definitions $definitions, ProcessRequest $instance, $this->tokenId = $token->getKey(); $this->elementId = $token->getProperty('element_ref'); $this->data = $data; + $this->attemptNum = $attemptNum; } /** From 0388eb1cb726fa93eea8b25ef7654745b8cf3c2a Mon Sep 17 00:00:00 2001 From: Nolan Ehrstrom Date: Mon, 3 Jul 2023 16:25:40 -0700 Subject: [PATCH 07/12] Remove debugging --- ProcessMaker/Jobs/ErrorHandling.php | 1 - 1 file changed, 1 deletion(-) diff --git a/ProcessMaker/Jobs/ErrorHandling.php b/ProcessMaker/Jobs/ErrorHandling.php index 6f900bdca9..bf4192c80f 100644 --- a/ProcessMaker/Jobs/ErrorHandling.php +++ b/ProcessMaker/Jobs/ErrorHandling.php @@ -73,7 +73,6 @@ private function requeue($job) $job->data, $job->attemptNum + 1 ); - \Log::info('Requeue the runScript process. Attempt ' . $job->attemptNum . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); $newJob->delay($this->retryWaitTime()); dispatch($newJob); } From fd644984bee910db56fc659cf222cdb15dd9d69d Mon Sep 17 00:00:00 2001 From: Roly Gutierrez Date: Wed, 5 Jul 2023 16:36:03 -0400 Subject: [PATCH 08/12] FOUR-8993 We added the new logic for handling retries of failed jobs to the service task. --- ProcessMaker/Jobs/BpmnAction.php | 3 - ProcessMaker/Jobs/ErrorHandling.php | 34 +++-------- ProcessMaker/Jobs/RunScriptTask.php | 17 +++++- ProcessMaker/Jobs/RunServiceTask.php | 59 ++++++++++++------- .../Models/ScriptDockerBindingFilesTrait.php | 2 +- ProcessMaker/Models/ScriptVersion.php | 4 +- ProcessMaker/Traits/MakeHttpRequests.php | 6 ++ 7 files changed, 71 insertions(+), 54 deletions(-) diff --git a/ProcessMaker/Jobs/BpmnAction.php b/ProcessMaker/Jobs/BpmnAction.php index 52a68439d7..70b870a92a 100644 --- a/ProcessMaker/Jobs/BpmnAction.php +++ b/ProcessMaker/Jobs/BpmnAction.php @@ -11,13 +11,10 @@ use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Log; -use Illuminate\Support\Facades\Notification; use ProcessMaker\BpmnEngine; use ProcessMaker\Models\Process as Definitions; use ProcessMaker\Models\ProcessRequest; use ProcessMaker\Models\ProcessRequestLock; -use ProcessMaker\Models\ProcessRequestToken; -use ProcessMaker\Notifications\ErrorExecutionNotification; use Throwable; abstract class BpmnAction implements ShouldQueue diff --git a/ProcessMaker/Jobs/ErrorHandling.php b/ProcessMaker/Jobs/ErrorHandling.php index bf4192c80f..6b369070e3 100644 --- a/ProcessMaker/Jobs/ErrorHandling.php +++ b/ProcessMaker/Jobs/ErrorHandling.php @@ -2,41 +2,25 @@ namespace ProcessMaker\Jobs; -use Illuminate\Support\Arr; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Notification; use ProcessMaker\Models\Process; use ProcessMaker\Models\ProcessRequest; use ProcessMaker\Models\ProcessRequestToken; -use ProcessMaker\Models\Script; use ProcessMaker\Models\ScriptVersion; use ProcessMaker\Notifications\ErrorExecutionNotification; class ErrorHandling { public $errorHandling = []; + + public static $callback; public function __construct( - public $element, - public $model, public $processRequestToken, - ) { - $errorHandling = json_decode($element->getProperty('errorHandling'), true) ?? []; - - if ($this->model instanceof ScriptVersion) { - $script = $this->model; - - if (!is_array($errorHandling) || empty($errorHandling)) { - $errorHandling = [ - 'timeout' => $script->timeout, - 'retry_attempts' => $script->retry_attempts, - 'retry_wait_time' => $script->retry_wait_time, - ]; - } - $this->errorHandling($errorHandling); - } - - // TODO data sources + ) + { + } public function handleRetries($job, $exception) @@ -45,13 +29,13 @@ public function handleRetries($job, $exception) if ($this->retryAttempts() > 0) { if ($job->attemptNum <= $this->retryAttempts()) { - Log::info('Retry the runScript process. Attempt ' . $job->attemptNum . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); + Log::info('Retry the job process. Attempt ' . $job->attemptNum . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); $this->requeue($job); return $message; } - $message = __('Script failed after :attempts total attempts', ['attempts' => $job->attemptNum]) . "\n" . $message; + $message = __('Job failed after :attempts total attempts', ['attempts' => $job->attemptNum - 1]) . "\n" . $message; $this->sendExecutionErrorNotification($message); @@ -86,7 +70,7 @@ public function sendExecutionErrorNotification(string $message) $user = $this->processRequestToken->processRequest->processVersion->manager; if ($user !== null) { Log::info('Send Execution Error Notification: ' . $message); - Notification::send($user, new ErrorExecutionNotification($this->processRequestToken, $message, $this->bpmnSetting)); + Notification::send($user, new ErrorExecutionNotification($this->processRequestToken, $message, $this->errorHandling)); } } } @@ -100,7 +84,7 @@ public function sendExecutionErrorNotification(string $message) */ public function errorHandling($value = null) { - \Log::info('errorHandling', ['value' => $value]); + Log::info('Setting errorHandling', ['value' => $value]); if (is_array($value) && !empty($value)) { $array = [ 'timeout', diff --git a/ProcessMaker/Jobs/RunScriptTask.php b/ProcessMaker/Jobs/RunScriptTask.php index abaf76b837..72e0eb7436 100644 --- a/ProcessMaker/Jobs/RunScriptTask.php +++ b/ProcessMaker/Jobs/RunScriptTask.php @@ -81,7 +81,22 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e $script = Script::findOrFail($scriptRef)->versionFor($instance); } - $errorHandling = new ErrorHandling($element, $script, $token); + $errorHandling = new ErrorHandling($token); + + /** + * This obtains the initial values and the values configured in the diagram; + * if the diagram values exist, it overwrites the initial values. + */ + $initial = [ + 'timeout' => $script->timeout, + 'retry_attempts' => $script->retry_attempts, + 'retry_wait_time' => $script->retry_wait_time, + ]; + $result = json_decode($element->getProperty('errorHandling'), true) ?? []; + if (is_array($result) && !empty($result)) { + $initial = array_merge($initial, $result); + } + $errorHandling->errorHandling($initial); $this->unlock(); $dataManager = new DataManager(); diff --git a/ProcessMaker/Jobs/RunServiceTask.php b/ProcessMaker/Jobs/RunServiceTask.php index ccc0c2e393..3a83bb49b0 100644 --- a/ProcessMaker/Jobs/RunServiceTask.php +++ b/ProcessMaker/Jobs/RunServiceTask.php @@ -29,6 +29,8 @@ class RunServiceTask extends BpmnAction implements ShouldQueue public $backoff = 60; + public $attemptNum; + /** * Create a new job instance. * @@ -37,7 +39,7 @@ class RunServiceTask extends BpmnAction implements ShouldQueue * @param \ProcessMaker\Models\ProcessRequestToken $token * @param array $data */ - public function __construct(Definitions $definitions, ProcessRequest $instance, ProcessRequestToken $token, array $data) + public function __construct(Definitions $definitions, ProcessRequest $instance, ProcessRequestToken $token, array $data, $attemptNum = 1) { if ($token->getOwner()) { $pmConfig = json_decode($token->getOwnerElement()->getProperty('config', '{}'), true); @@ -50,6 +52,7 @@ public function __construct(Definitions $definitions, ProcessRequest $instance, $this->tokenId = $token->getKey(); $this->elementId = $token->getProperty('element_ref'); $this->data = $data; + $this->attemptNum = $attemptNum; } /** @@ -65,7 +68,6 @@ public function action(ProcessRequestToken $token = null, ServiceTaskInterface $ } $implementation = $element->getImplementation(); $configuration = json_decode($element->getProperty('config'), true); - $errorHandling = json_decode($element->getProperty('errorHandling'), true) ?? []; // Check to see if we've failed parsing. If so, let's convert to empty array. if ($configuration === null) { @@ -84,19 +86,40 @@ public function action(ProcessRequestToken $token = null, ServiceTaskInterface $ throw new ScriptException('Service task not implemented: ' . $implementation); } - //todo: It is necessary to obtain the configuration values of the "dataSource" for the default values. - $this->errorHandling($errorHandling); + $errorHandling = new ErrorHandling($token); + + /** + * This obtains the initial values and the values configured in the diagram; + * if the diagram values exist, it overwrites the initial values. + */ + ErrorHandling::$callback = function ($params) use ($element, $errorHandling) { + $initial = [ + 'timeout' => $params['timeout'], + 'retry_attempts' => $params['retry_attempts'], + 'retry_wait_time' => $params['retry_wait_time'], + ]; + + $result = json_decode($element->getProperty('errorHandling'), true) ?? []; + if (is_array($result) && !empty($result)) { + $initial = array_merge($initial, $result); + } + $errorHandling->errorHandling($initial); + }; $this->unlock(); $dataManager = new DataManager(); $data = $dataManager->getData($token); + /** + * todo: Please review the "else" section as it appears unreachable since if `$existsImpl` + * is not true, an exception is thrown a few lines above. + */ if ($existsImpl) { $response = [ 'output' => WorkflowManager::runServiceImplementation($implementation, $data, $configuration, $token->getId()), ]; } else { - $response = $script->runScript($data, $configuration, $token->getId(), $errorHandling); + $response = $script->runScript($data, $configuration, $token->getId(), $errorHandling->timeout()); } $this->withUpdatedContext(function ($engine, $instance, $element, $processModel, $token) use ($response) { // Exit if the task was completed or closed @@ -118,27 +141,19 @@ public function action(ProcessRequestToken $token = null, ServiceTaskInterface $ } catch (Throwable $exception) { // Change to error status $token->setStatus(ServiceTaskInterface::TOKEN_STATE_FAILING); + + $message = $errorHandling->handleRetries($this, $exception); + $error = $element->getRepository()->createError(); $error->setName($exception->getMessage()); + $token->setProperty('error', $error); - Log::info('Service task failed: ' . $implementation . ' - ' . $exception->getMessage()); + $exceptionClass = get_class($exception); + $modifiedException = new $exceptionClass($message); + $token->logError($modifiedException, $element); + + Log::error('Service task failed: ' . $implementation . ' - ' . $exception->getMessage()); Log::error($exception->getTraceAsString()); - - if ($this->retryAttempts() > 0) { - if ($this->attempts() <= $this->retryAttempts()) { - Log::info('Retry the runScript process. Attempt ' . $this->attempts() . ' of ' . $this->retryAttempts() . ', Wait time: ' . $this->retryWaitTime()); - $this->release($this->retryWaitTime()); - return; - } - $message = __('Failed after :num total attempts', ['num' => $this->attempts()]); - $message = $message . "\n" . $exception->getMessage(); - - $this->sendExecutionErrorNotification($message, $token->getId(), $errorHandling); - throw $exception; - } else { - $this->sendExecutionErrorNotification($exception->getMessage(), $token->getId(), $errorHandling); - throw $exception; - } } } diff --git a/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php b/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php index 201061bc1e..2e84c92548 100644 --- a/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php +++ b/ProcessMaker/Models/ScriptDockerBindingFilesTrait.php @@ -83,7 +83,7 @@ private function runContainer($image, $command, $parameters, $bindings, $timeout . implode("\n", $output) ); } - Log::error('Script threw return code ' . $returnCode . 'Message: ' . implode("\n", $output)); + Log::error('Script threw return code ' . $returnCode . ' Message: ' . implode("\n", $output)); $message = implode("\n", $output); $message .= "\n\nProcessMaker Stack:\n"; diff --git a/ProcessMaker/Models/ScriptVersion.php b/ProcessMaker/Models/ScriptVersion.php index 1b79b9def3..1fb3674f19 100644 --- a/ProcessMaker/Models/ScriptVersion.php +++ b/ProcessMaker/Models/ScriptVersion.php @@ -50,7 +50,7 @@ public function parent() * @param array $data * @param array $config */ - public function runScript(array $data, array $config, $tokenId = '', $errorHandling = []) + public function runScript(array $data, array $config, $tokenId = '') { $script = $this->parent->replicate(); $except = $script->getGuarded(); @@ -58,7 +58,7 @@ public function runScript(array $data, array $config, $tokenId = '', $errorHandl $script->$prop = $this->$prop; } - return $script->runScript($data, $config, $tokenId, $errorHandling); + return $script->runScript($data, $config, $tokenId); } /** diff --git a/ProcessMaker/Traits/MakeHttpRequests.php b/ProcessMaker/Traits/MakeHttpRequests.php index 129fa8ace8..6abce6288e 100644 --- a/ProcessMaker/Traits/MakeHttpRequests.php +++ b/ProcessMaker/Traits/MakeHttpRequests.php @@ -14,6 +14,7 @@ use ProcessMaker\Exception\HttpInvalidArgumentException; use ProcessMaker\Exception\HttpResponseException; use ProcessMaker\Helpers\StringHelper; +use ProcessMaker\Jobs\ErrorHandling; use ProcessMaker\Models\FormalExpression; use Psr\Http\Message\ResponseInterface; @@ -202,6 +203,11 @@ private function prepareRequestWithOutboundConfig(array $requestData, array &$co $request = [$method, $url, $headers, $body, $bodyType]; $request = $this->addAuthorizationHeaders(...$request); //todo: The values of $endpoint must be passed somehow to RunServiceTask inside action method. + + if (is_callable(ErrorHandling::$callback)) { + $fn = ErrorHandling::$callback; + $fn($endpoint); + } return $request; } From d549992e4a3f83c1454199e4498d55b12817f28d Mon Sep 17 00:00:00 2001 From: Nolan Ehrstrom Date: Wed, 5 Jul 2023 18:14:46 -0700 Subject: [PATCH 09/12] Update error handling --- ProcessMaker/Jobs/ErrorHandling.php | 150 ++++++++++++------ ProcessMaker/Jobs/RunScriptTask.php | 18 +-- ProcessMaker/Jobs/RunServiceTask.php | 31 +--- .../Nayra/Managers/WorkflowManagerDefault.php | 4 +- ProcessMaker/Traits/MakeHttpRequests.php | 13 +- 5 files changed, 117 insertions(+), 99 deletions(-) diff --git a/ProcessMaker/Jobs/ErrorHandling.php b/ProcessMaker/Jobs/ErrorHandling.php index 6b369070e3..3fabe53999 100644 --- a/ProcessMaker/Jobs/ErrorHandling.php +++ b/ProcessMaker/Jobs/ErrorHandling.php @@ -2,6 +2,7 @@ namespace ProcessMaker\Jobs; +use Illuminate\Support\Arr; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Notification; use ProcessMaker\Models\Process; @@ -12,15 +13,30 @@ class ErrorHandling { - public $errorHandling = []; - - public static $callback; + /** + * Error handling settings that are set in modeler. + * + * These settings take precedence over those set in script or + * data source configuration. + * + * @var array + */ + public $bpmnErrorHandling = []; + + /** + * Default settings that are set in script or data source configuration. + * + * $bpmnErrorHandling takes precedence over these if set. + * + * @var array + */ + public $defaultErrorHandling = []; public function __construct( + public $element, public $processRequestToken, - ) - { - + ) { + $this->bpmnErrorHandling = json_decode($element->getProperty('errorHandling'), true) ?? []; } public function handleRetries($job, $exception) @@ -35,11 +51,9 @@ public function handleRetries($job, $exception) return $message; } - $message = __('Job failed after :attempts total attempts', ['attempts' => $job->attemptNum - 1]) . "\n" . $message; + $message = __('Job failed after :attempts total attempts', ['attempts' => $job->attemptNum]) . "\n" . $message; $this->sendExecutionErrorNotification($message); - - return $message; } else { $this->sendExecutionErrorNotification($message); } @@ -70,70 +84,114 @@ public function sendExecutionErrorNotification(string $message) $user = $this->processRequestToken->processRequest->processVersion->manager; if ($user !== null) { Log::info('Send Execution Error Notification: ' . $message); - Notification::send($user, new ErrorExecutionNotification($this->processRequestToken, $message, $this->errorHandling)); + Notification::send($user, new ErrorExecutionNotification($this->processRequestToken, $message, $this->bpmnErrorHandling)); } } } /** - * This sets the property errorHandling, and if the parameter doesn't exist, - * it returns the current value of property errorHandling. + * Get the effective timeout * - * @param mixed $value - * @return mixed + * @return int */ - public function errorHandling($value = null) + public function timeout() { - Log::info('Setting errorHandling', ['value' => $value]); - if (is_array($value) && !empty($value)) { - $array = [ - 'timeout', - 'retry_attempts', - 'retry_wait_time', - ]; - foreach ($array as $val) { - $digit = 0; - if (is_string($value[$val]) && ctype_digit($value[$val])) { - $digit = intval($value[$val]); - } - if (is_int($value[$val])) { - $digit = $value[$val]; - } - $value[$val] = $digit; - } - $this->errorHandling = $value; - } - - return $this->errorHandling; + return $this->get('timeout'); } /** - * This retrieves the value of errorHandling['timeout']. + * Get the effective retry attempts * * @return int */ - public function timeout() + public function retryAttempts() { - return $this->errorHandling['timeout']; + return $this->get('retry_attempts'); } /** - * This retrieves the value of errorHandling['retry_attempts']. + * Get the effective retry wait time. * * @return int */ - public function retryAttempts() + public function retryWaitTime() { - return $this->errorHandling['retry_attempts']; + return $this->get('retry_wait_time'); } /** - * This retrieves the value of errorHandling['retry_wait_time']. + * Get the attribute from the bpmnErrorHandling array but if it's not set + * return the defaultErrorHandling value * - * @return int + * @param string $attribute + * @return void */ - public function retryWaitTime() + public function get($attribute) + { + $value = Arr::get( + $this->bpmnErrorHandling, + $attribute, + null + ); + + if ($value === null || $value === '') { + $value = Arr::get($this->defaultErrorHandling, $attribute, 0); + } + + return (int) $value; + } + + /** + * Set defaults from script model + * + * @param ScriptVersion $script + * @return void + */ + public function setDefaultsFromScript(ScriptVersion $script) { - return $this->errorHandling['retry_wait_time']; + $this->defaultErrorHandling = [ + 'timeout' => $script->timeout, + 'retry_attempts' => $script->retry_attempts, + 'retry_wait_time' => $script->retry_wait_time, + ]; + } + + /** + * Set defaults from data source model endpoint + * + * @param array $config + * @return void + */ + public function setDefaultsFromDataSourceConfig(array $config) + { + // If this is not a Data Connecter, don't do any error handling + $id = Arr::get($config, 'dataSource', null); + if (!$id) { + return; + } + + // Check if the data source package is installed + $class = "ProcessMaker\Packages\Connectors\DataSources\Models\DataSource"; + if (!class_exists($class)) { + return; + } + + // Check if the data source exists + $dataSource = $class::find($id); + if (!$dataSource) { + return; + } + + // Check if the endpoint config exists in the data source + $endpointConfig = Arr::get($dataSource->endpoints, Arr::get($config, 'endpoint', null)); + if (!$endpointConfig) { + return; + } + + $this->defaultErrorHandling = [ + 'timeout' => Arr::get($endpointConfig, 'timeout', 0), + 'retry_attempts' => Arr::get($endpointConfig, 'retry_attempts', 0), + 'retry_wait_time' => Arr::get($endpointConfig, 'retry_wait_time', 5), + ]; } } diff --git a/ProcessMaker/Jobs/RunScriptTask.php b/ProcessMaker/Jobs/RunScriptTask.php index 72e0eb7436..130c69cf95 100644 --- a/ProcessMaker/Jobs/RunScriptTask.php +++ b/ProcessMaker/Jobs/RunScriptTask.php @@ -81,22 +81,8 @@ public function action(ProcessRequestToken $token = null, ScriptTaskInterface $e $script = Script::findOrFail($scriptRef)->versionFor($instance); } - $errorHandling = new ErrorHandling($token); - - /** - * This obtains the initial values and the values configured in the diagram; - * if the diagram values exist, it overwrites the initial values. - */ - $initial = [ - 'timeout' => $script->timeout, - 'retry_attempts' => $script->retry_attempts, - 'retry_wait_time' => $script->retry_wait_time, - ]; - $result = json_decode($element->getProperty('errorHandling'), true) ?? []; - if (is_array($result) && !empty($result)) { - $initial = array_merge($initial, $result); - } - $errorHandling->errorHandling($initial); + $errorHandling = new ErrorHandling($element, $token); + $errorHandling->setDefaultsFromScript($script); $this->unlock(); $dataManager = new DataManager(); diff --git a/ProcessMaker/Jobs/RunServiceTask.php b/ProcessMaker/Jobs/RunServiceTask.php index 3a83bb49b0..3fdc74845f 100644 --- a/ProcessMaker/Jobs/RunServiceTask.php +++ b/ProcessMaker/Jobs/RunServiceTask.php @@ -86,37 +86,20 @@ public function action(ProcessRequestToken $token = null, ServiceTaskInterface $ throw new ScriptException('Service task not implemented: ' . $implementation); } - $errorHandling = new ErrorHandling($token); - - /** - * This obtains the initial values and the values configured in the diagram; - * if the diagram values exist, it overwrites the initial values. - */ - ErrorHandling::$callback = function ($params) use ($element, $errorHandling) { - $initial = [ - 'timeout' => $params['timeout'], - 'retry_attempts' => $params['retry_attempts'], - 'retry_wait_time' => $params['retry_wait_time'], - ]; - - $result = json_decode($element->getProperty('errorHandling'), true) ?? []; - if (is_array($result) && !empty($result)) { - $initial = array_merge($initial, $result); - } - $errorHandling->errorHandling($initial); - }; + $errorHandling = new ErrorHandling($element, $token); + $errorHandling->setDefaultsFromDataSourceConfig($configuration); $this->unlock(); $dataManager = new DataManager(); $data = $dataManager->getData($token); /** - * todo: Please review the "else" section as it appears unreachable since if `$existsImpl` + * todo: Please review the "else" section as it appears unreachable since if `$existsImpl` * is not true, an exception is thrown a few lines above. */ if ($existsImpl) { $response = [ - 'output' => WorkflowManager::runServiceImplementation($implementation, $data, $configuration, $token->getId()), + 'output' => WorkflowManager::runServiceImplementation($implementation, $data, $configuration, $token->getId(), $errorHandling->timeout()), ]; } else { $response = $script->runScript($data, $configuration, $token->getId(), $errorHandling->timeout()); @@ -167,12 +150,6 @@ public function failed(Throwable $exception) return; } - if (get_class($exception) === "Illuminate\\Queue\\MaxAttemptsExceededException") { - $message = 'This is a type MaxAttemptsExceededException exception, it appears ' - . 'that the global value configured in config/horizon.php has been exceeded ' - . 'in the retries. Please consult with your main administrator.'; - Log::error($message); - } Log::error('Script (#' . $this->tokenId . ') failed: ' . $exception->getMessage()); Log::error($exception->getTraceAsString()); $token = ProcessRequestToken::find($this->tokenId); diff --git a/ProcessMaker/Nayra/Managers/WorkflowManagerDefault.php b/ProcessMaker/Nayra/Managers/WorkflowManagerDefault.php index 3ef036412b..b97c351546 100644 --- a/ProcessMaker/Nayra/Managers/WorkflowManagerDefault.php +++ b/ProcessMaker/Nayra/Managers/WorkflowManagerDefault.php @@ -357,11 +357,11 @@ class_exists($this->serviceTaskImplementations[$implementation]); * * @return mixed */ - public function runServiceImplementation($implementation, array $data, array $config, $tokenId = '') + public function runServiceImplementation($implementation, array $data, array $config, $tokenId = '', $timeout = 0) { $class = $this->serviceTaskImplementations[$implementation]; $service = new $class(); - return $service->run($data, $config, $tokenId); + return $service->run($data, $config, $tokenId, $timeout); } } diff --git a/ProcessMaker/Traits/MakeHttpRequests.php b/ProcessMaker/Traits/MakeHttpRequests.php index 6abce6288e..dec3c38140 100644 --- a/ProcessMaker/Traits/MakeHttpRequests.php +++ b/ProcessMaker/Traits/MakeHttpRequests.php @@ -35,6 +35,8 @@ trait MakeHttpRequests private $mustache = null; + private $timeout = 0; + private function getMustache() { if ($this->mustache === null) { @@ -202,12 +204,6 @@ private function prepareRequestWithOutboundConfig(array $requestData, array &$co $request = [$method, $url, $headers, $body, $bodyType]; $request = $this->addAuthorizationHeaders(...$request); - //todo: The values of $endpoint must be passed somehow to RunServiceTask inside action method. - - if (is_callable(ErrorHandling::$callback)) { - $fn = ErrorHandling::$callback; - $fn($endpoint); - } return $request; } @@ -436,8 +432,9 @@ private function call($method, $url, array $headers, $body, $bodyType) { $client = $this->client ?? app()->make(Client::class, [ 'config' => [ - 'verify' => $this->verifySsl - ] + 'verify' => $this->verifySsl, + 'timeout' => $this->timeout, + ], ]); $options = []; if ($bodyType === 'form-data') { From c260cdabd2d0441093d1edd619766b069fcb8787 Mon Sep 17 00:00:00 2001 From: Nolan Ehrstrom Date: Fri, 7 Jul 2023 14:03:58 -0700 Subject: [PATCH 10/12] Fix test --- ProcessMaker/Jobs/RunServiceTask.php | 4 +- tests/Jobs/ErrorHandlingTest.php | 56 +++++++++++++++++++++++----- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/ProcessMaker/Jobs/RunServiceTask.php b/ProcessMaker/Jobs/RunServiceTask.php index 3fdc74845f..72ce729918 100644 --- a/ProcessMaker/Jobs/RunServiceTask.php +++ b/ProcessMaker/Jobs/RunServiceTask.php @@ -128,14 +128,14 @@ public function action(ProcessRequestToken $token = null, ServiceTaskInterface $ $message = $errorHandling->handleRetries($this, $exception); $error = $element->getRepository()->createError(); - $error->setName($exception->getMessage()); + $error->setName($message); $token->setProperty('error', $error); $exceptionClass = get_class($exception); $modifiedException = new $exceptionClass($message); $token->logError($modifiedException, $element); - Log::error('Service task failed: ' . $implementation . ' - ' . $exception->getMessage()); + Log::error('Service task failed: ' . $implementation . ' - ' . $message); Log::error($exception->getTraceAsString()); } } diff --git a/tests/Jobs/ErrorHandlingTest.php b/tests/Jobs/ErrorHandlingTest.php index 61bb918581..bfe8f2d2e1 100644 --- a/tests/Jobs/ErrorHandlingTest.php +++ b/tests/Jobs/ErrorHandlingTest.php @@ -2,9 +2,11 @@ namespace Tests\Jobs; +use Illuminate\Support\Facades\Queue; use Mockery; use ProcessMaker\Jobs\ErrorHandling; use ProcessMaker\Jobs\RunScriptTask; +use ProcessMaker\Models\Process; use ProcessMaker\Models\ProcessRequest; use ProcessMaker\Models\ProcessRequestToken; use ProcessMaker\Models\Script; @@ -14,20 +16,37 @@ class ErrorHandlingTest extends TestCase { private function runAssertions($settings) { + Queue::fake(); + extract($settings); $errorHandling = ['timeout' => $bpmnTimeout, 'retry_attempts' => $bpmnRetryAttempts, 'retry_wait_time' => $bpmnRetryWaitTime]; $element = Mockery::mock(); $element->shouldReceive('getProperty')->andReturn(json_encode($errorHandling)); - $job = Mockery::mock(RunScriptTask::class); - $job->shouldReceive('attempts')->andReturn($attempt); - $job->shouldReceive('release')->with($expectedWaitTime)->times($expectedReleaseCount); $script = Script::factory()->create(['timeout' => $modelTimeout, 'retry_attempts' => $modelRetryAttempts, 'retry_wait_time' => $modelRetryWaitTime]); - $token = ProcessRequestToken::factory()->create(); - $errorHandling = new ErrorHandling($element, $script, $token); + $process = Process::factory()->create(); + $processRequest = ProcessRequest::factory()->create([ + 'process_id' => $process->id, + ]); + $token = ProcessRequestToken::factory()->create([ + 'process_request_id' => $processRequest->id, + ]); + + $job = new RunScriptTask($process, $processRequest, $token, [], $attempt); + $errorHandling = new ErrorHandling($element, $token); + $errorHandling->setDefaultsFromScript($script->getLatestVersion()); $this->assertEquals($expectedTimeout, $errorHandling->timeout()); $errorHandling->handleRetries($job, new \RuntimeException('error')); + + if ($expectedNextAttempt !== false) { + Queue::assertPushed(RunScriptTask::class, function ($job) use ($expectedNextAttempt, $expectedWaitTime) { + return $job->attemptNum === $expectedNextAttempt && + $job->delay === $expectedWaitTime; + }); + } else { + Queue::assertNotPushed(RunScriptTask::class); + } } public function testRetry() @@ -35,7 +54,7 @@ public function testRetry() $this->runAssertions([ 'attempt' => 3, 'expectedTimeout' => 15, - 'expectedReleaseCount' => 1, // retry the job + 'expectedNextAttempt' => 4, // retry the job 'expectedWaitTime' => 5, 'modelTimeout' => 8, @@ -47,12 +66,29 @@ public function testRetry() ]); } + public function testRetryUseModelSettings() + { + $this->runAssertions([ + 'attempt' => 3, + 'expectedTimeout' => 15, + 'expectedNextAttempt' => 4, // retry the job + 'expectedWaitTime' => 5, + + 'modelTimeout' => 8, + 'modelRetryAttempts' => 99, + 'modelRetryWaitTime' => 6, + 'bpmnTimeout' => 15, + 'bpmnRetryAttempts' => '', // use modelRetryAttempts + 'bpmnRetryWaitTime' => 5, + ]); + } + public function testDoNotRetry() { $this->runAssertions([ 'attempt' => 4, 'expectedTimeout' => 15, - 'expectedReleaseCount' => 0, // do not retry the job because attempt is 4 and bpmnRetryAttempts is 3 + 'expectedNextAttempt' => false, // do not retry the job because attempt is 4 and bpmnRetryAttempts is 3 'expectedWaitTime' => 5, 'modelTimeout' => 8, @@ -67,9 +103,9 @@ public function testDoNotRetry() public function testRetryWaitFromModel() { $this->runAssertions([ - 'attempt' => 4, + 'attempt' => 3, 'expectedTimeout' => 15, - 'expectedReleaseCount' => 0, + 'expectedNextAttempt' => 4, 'expectedWaitTime' => 6, // use modelRetryWaitTime because bpmnWaitTime is empty 'modelTimeout' => 8, @@ -86,7 +122,7 @@ public function testTimeoutFromModel() $this->runAssertions([ 'attempt' => 3, 'expectedTimeout' => 8, // use modelTimeout because bpmnTimeout is empty - 'expectedReleaseCount' => 1, + 'expectedNextAttempt' => 4, 'expectedWaitTime' => 5, 'modelTimeout' => 8, From a3ef76b68099b824ec67cb82cefe282d7f0380b8 Mon Sep 17 00:00:00 2001 From: Nolan Ehrstrom Date: Fri, 7 Jul 2023 15:03:21 -0700 Subject: [PATCH 11/12] Remove test thats no longer relevant --- tests/Model/ScriptTest.php | 86 -------------------------------------- 1 file changed, 86 deletions(-) delete mode 100644 tests/Model/ScriptTest.php diff --git a/tests/Model/ScriptTest.php b/tests/Model/ScriptTest.php deleted file mode 100644 index 9af426c314..0000000000 --- a/tests/Model/ScriptTest.php +++ /dev/null @@ -1,86 +0,0 @@ -mockWithExpectedTimeout(123); - - $script = Script::factory()->create([ - 'code' => 'foo', - 'timeout' => 5, - 'retry_attempts' => 3, - 'retry_wait_time' => 1, - ]); - $script->runScript([], [], '', ['timeout' => 123]); - } - - public function testUseTimeoutFromSetting() - { - $this->mockWithExpectedTimeout(5); - - $script = Script::factory()->create([ - 'code' => 'foo', - 'timeout' => 5, - 'retry_attempts' => 3, - 'retry_wait_time' => 1, - ]); - $script->runScript([], [], '', ['timeout' => '']); - } - - private function mockWithExpectedTimeout($timeout) - { - $mock = Mockery::mock(MockRunner::class); - $mock->shouldReceive('run')->once()->with('foo', [], [], $timeout, Mockery::any()); - $mock->shouldReceive('setTokenId')->once(); - app()->bind(MockRunner::class, function () use ($mock) { - return $mock; - }); - } - - /** - * This test performs a test of retry_attempts and the retry_wait_time for script execution. - */ - public function testUseRetryAttemptsAndRetryWaitTime() - { - //Mockery needs to know about this retry. - $times = 3; - $this->mockWithExpectedTimeoutWithException(2, $times + 1); // Add 1 for original call - - $this->expectException(ScriptException::class); - - $script = Script::factory()->create([ - 'code' => 'foo', - 'timeout' => 2, - 'retry_attempts' => $times, - 'retry_wait_time' => 1, - ]); - $script->runScript([], [], '', ['timeout' => '']); - } - - /** - * It is necessary to pass this value to Mockery because we are attempting retries. Mockery needs - * to know about this retry. - */ - private function mockWithExpectedTimeoutWithException($timeout, $times) - { - $mock = Mockery::mock(MockRunner::class); - $mock->shouldReceive('setTokenId')->times($times); - $mock->shouldReceive('run')->times($times)->with('foo', [], [], $timeout, Mockery::any())->andReturnUsing(function () { - throw new \RuntimeException('Error occurred'); - }); - - app()->bind(MockRunner::class, function () use ($mock) { - return $mock; - }); - } -} From 7b6a374616b5ab192e765606804bc8005fc85dd4 Mon Sep 17 00:00:00 2001 From: Nolan Ehrstrom Date: Fri, 7 Jul 2023 15:11:04 -0700 Subject: [PATCH 12/12] Allow Script or Script Version for setting defaults --- ProcessMaker/Jobs/ErrorHandling.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ProcessMaker/Jobs/ErrorHandling.php b/ProcessMaker/Jobs/ErrorHandling.php index 3fabe53999..7343c6452f 100644 --- a/ProcessMaker/Jobs/ErrorHandling.php +++ b/ProcessMaker/Jobs/ErrorHandling.php @@ -8,6 +8,7 @@ use ProcessMaker\Models\Process; use ProcessMaker\Models\ProcessRequest; use ProcessMaker\Models\ProcessRequestToken; +use ProcessMaker\Models\Script; use ProcessMaker\Models\ScriptVersion; use ProcessMaker\Notifications\ErrorExecutionNotification; @@ -147,7 +148,7 @@ public function get($attribute) * @param ScriptVersion $script * @return void */ - public function setDefaultsFromScript(ScriptVersion $script) + public function setDefaultsFromScript(Script|ScriptVersion $script) { $this->defaultErrorHandling = [ 'timeout' => $script->timeout,