From fe2fc374aa0b4f1a6631047651511af98445da13 Mon Sep 17 00:00:00 2001 From: Omar Brikaa Date: Fri, 15 Sep 2023 16:48:35 +0300 Subject: [PATCH 1/5] 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/5] 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/5] 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 } } }); From fef00b96f169c1b1508d871ae901359d2ced82d2 Mon Sep 17 00:00:00 2001 From: Omar Brikaa Date: Tue, 3 Oct 2023 13:59:23 +0300 Subject: [PATCH 4/5] Improve containers stopping performance by handling SIGTERM --- api/src/index.js | 12 ++++++++++-- repo/Dockerfile | 2 +- repo/entrypoint.sh | 6 +++--- repo/serve.py | 18 ++++++++++++++++++ 4 files changed, 32 insertions(+), 6 deletions(-) create mode 100644 repo/serve.py diff --git a/api/src/index.js b/api/src/index.js index 8a21e57..6ad9390 100644 --- a/api/src/index.js +++ b/api/src/index.js @@ -35,7 +35,10 @@ expressWs(app); } } }); - fss.chmodSync(path.join(config.data_directory, globals.data_directories.jobs), 0o711) + fss.chmodSync( + path.join(config.data_directory, globals.data_directories.jobs), + 0o711 + ); logger.info('Loading packages'); const pkgdir = path.join( @@ -92,7 +95,12 @@ expressWs(app); logger.debug('Calling app.listen'); const [address, port] = config.bind_address.split(':'); - app.listen(port, address, () => { + const server = app.listen(port, address, () => { logger.info('API server started on', config.bind_address); }); + + process.on('SIGTERM', () => { + server.close(); + process.exit(0) + }); })(); diff --git a/repo/Dockerfile b/repo/Dockerfile index fe61a6f..86be49a 100644 --- a/repo/Dockerfile +++ b/repo/Dockerfile @@ -14,7 +14,7 @@ RUN apt-get update && apt-get install -y unzip autoconf build-essential libssl-d rm -rf /var/lib/apt/lists/* && \ update-alternatives --install /usr/bin/python python /usr/bin/python3.7 2 -ADD entrypoint.sh mkindex.sh / +ADD entrypoint.sh mkindex.sh serve.py / ENTRYPOINT ["bash","/entrypoint.sh"] CMD ["--no-build"] diff --git a/repo/entrypoint.sh b/repo/entrypoint.sh index 6c47e37..1565143 100755 --- a/repo/entrypoint.sh +++ b/repo/entrypoint.sh @@ -27,7 +27,7 @@ do echo "Done with package $pkg" elif [[ $CI -eq 1 ]]; then echo "Commit SHA: $pkg" - + cd .. echo "Changed files:" git diff --name-only $pkg^1 $pkg @@ -52,8 +52,8 @@ echo "Index created" if [[ $SERVER -eq 1 ]]; then echo "Starting index server.." - python3 -m http.server + exec python3 /serve.py else echo "Skipping starting index server" fi -exit 0 \ No newline at end of file +exit 0 diff --git a/repo/serve.py b/repo/serve.py new file mode 100644 index 0000000..a821219 --- /dev/null +++ b/repo/serve.py @@ -0,0 +1,18 @@ +import signal +import sys +import http.server +import socketserver + +PORT = 8000 + +Handler = http.server.SimpleHTTPRequestHandler + + +def signal_handler(sig, frame): + sys.exit(0) + +signal.signal(signal.SIGTERM, signal_handler) + +with socketserver.TCPServer(("", PORT), Handler) as httpd: + print("serving at port", PORT) + httpd.serve_forever() From 016a8c086f7c9cc9d785f8ccc2ffb6975cb4a4da Mon Sep 17 00:00:00 2001 From: Omar Brikaa Date: Tue, 3 Oct 2023 15:21:48 +0300 Subject: [PATCH 5/5] exec comment --- repo/entrypoint.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/repo/entrypoint.sh b/repo/entrypoint.sh index 1565143..c167463 100755 --- a/repo/entrypoint.sh +++ b/repo/entrypoint.sh @@ -52,6 +52,7 @@ echo "Index created" if [[ $SERVER -eq 1 ]]; then echo "Starting index server.." + # We want the child process to replace the shell to handle signals exec python3 /serve.py else echo "Skipping starting index server"