From fe2fc374aa0b4f1a6631047651511af98445da13 Mon Sep 17 00:00:00 2001 From: Omar Brikaa Date: Fri, 15 Sep 2023 16:48:35 +0300 Subject: [PATCH 1/3] Improve normal execution error handling - Properly differentiate between bad requests and internal server errors - Avoid clean up evasion by putting the cleanup in the finally block --- api/src/api/v2.js | 20 +++++++++++++++----- packages/bash/5.2.0/build.sh | 0 2 files changed, 15 insertions(+), 5 deletions(-) mode change 100644 => 100755 packages/bash/5.2.0/build.sh diff --git a/api/src/api/v2.js b/api/src/api/v2.js index 3dfa3ee..ad07494 100644 --- a/api/src/api/v2.js +++ b/api/src/api/v2.js @@ -265,9 +265,13 @@ router.ws('/connect', async (ws, req) => { }); router.post('/execute', async (req, res) => { + let job; + try { + job = await get_job(req.body); + } catch (error) { + return res.status(400).json(error); + } try { - const job = await get_job(req.body); - await job.prime(); let result = await job.execute(); @@ -276,11 +280,17 @@ router.post('/execute', async (req, res) => { result.run = result.compile; } - await job.cleanup(); - return res.status(200).send(result); } catch (error) { - return res.status(400).json(error); + logger.error(`Error executing job: ${job.uuid}:\n${error}`); + return res.status(500).send(); + } finally { + try { + await job.cleanup(); + } catch (error) { + logger.error(`Error cleaning up job: ${job.uuid}:\n${error}`); + return res.status(500).send(); + } } }); diff --git a/packages/bash/5.2.0/build.sh b/packages/bash/5.2.0/build.sh old mode 100644 new mode 100755 From 040e19fdc2e03e241519e81d5bd7b4c866cf8b6b Mon Sep 17 00:00:00 2001 From: Omar Brikaa Date: Fri, 15 Sep 2023 20:39:15 +0300 Subject: [PATCH 2/3] Interactive execution: run job cleanup regardless of errors --- api/src/api/v2.js | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/api/src/api/v2.js b/api/src/api/v2.js index ad07494..1b015b5 100644 --- a/api/src/api/v2.js +++ b/api/src/api/v2.js @@ -210,19 +210,26 @@ router.ws('/connect', async (ws, req) => { if (job === null) { job = await get_job(msg); - await job.prime(); + try { + await job.prime(); - ws.send( - JSON.stringify({ - type: 'runtime', - language: job.runtime.language, - version: job.runtime.version.raw, - }) - ); - - await job.execute(event_bus); - await job.cleanup(); + ws.send( + JSON.stringify({ + type: 'runtime', + language: job.runtime.language, + version: job.runtime.version.raw, + }) + ); + await job.execute(event_bus); + } catch (error) { + logger.error( + `Error cleaning up job: ${job.uuid}:\n${error}` + ); + throw error; + } finally { + await job.cleanup(); + } ws.close(4999, 'Job Completed'); } else { ws.close(4000, 'Already Initialized'); From 6a47869578b89212c0e7f2a37d395e804c924d81 Mon Sep 17 00:00:00 2001 From: Omar Brikaa Date: Sat, 16 Sep 2023 21:37:09 +0300 Subject: [PATCH 3/3] Comments explaining the try-catch flow --- api/src/api/v2.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/api/v2.js b/api/src/api/v2.js index 1b015b5..032fd51 100644 --- a/api/src/api/v2.js +++ b/api/src/api/v2.js @@ -230,7 +230,7 @@ router.ws('/connect', async (ws, req) => { } finally { await job.cleanup(); } - ws.close(4999, 'Job Completed'); + ws.close(4999, 'Job Completed'); // Will not execute if an error is thrown above } else { ws.close(4000, 'Already Initialized'); } @@ -293,10 +293,10 @@ router.post('/execute', async (req, res) => { return res.status(500).send(); } finally { try { - await job.cleanup(); + await job.cleanup(); // This gets executed before the returns in try/catch } catch (error) { logger.error(`Error cleaning up job: ${job.uuid}:\n${error}`); - return res.status(500).send(); + return res.status(500).send(); // On error, this replaces the return in the outer try-catch } } });