Skip to content

Commit 68cbd6a

Browse files
Ensure process termination occurs in a reasonable timeframe []
1 parent 30cbe88 commit 68cbd6a

4 files changed

Lines changed: 103 additions & 6 deletions

File tree

‎dev_tools/pylibs/edgacknowledg/forge/workspace.py‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
from typing import Iterable, Optional, Sequence
4343

4444
import edgtest
45+
import edgutil
4546

4647
from edgacknowledg.forge import Forge, PullRequestInfo
4748
from edgacknowledg.forge.runner.roles import BUILD_STAGING_DIR, REVIEWS_DIR
@@ -75,6 +76,9 @@
7576
# reads all of that directory into memory, before we can count.
7677
MAX_ARTIFACT_ENTRIES = 100_000
7778
MAX_CENTRAL_DIRECTORY_BYTES = 16 * 1024 * 1024
79+
# Seconds a git process being stopped has to clean up (e.g. remove its lock
80+
# files) before it is killed; well within docker stop's default 10.
81+
_TERMINATE_ALLOWANCE = 5.0
7882

7983

8084
class WorkspaceError(Exception):
@@ -678,7 +682,9 @@ async def collect_garbage(self, repository: str, open_count: int) -> None:
678682
async def _communicate_bounded(process: asyncio.subprocess.Process,
679683
limit: int) -> Optional[tuple[bytes, bytes]]:
680684
'''``process``'s output (stdout, stderr), once it exits; ``None``, having
681-
killed it, should its stdout be over ``limit`` bytes.
685+
stopped it, should its stdout be over ``limit`` bytes. Should this be
686+
cancelled (or fail), it is stopped too, rather than left writing to a
687+
pipe no one reads.
682688
'''
683689
assert process.stdout is not None and process.stderr is not None
684690
errors = asyncio.create_task(process.stderr.read())
@@ -688,13 +694,17 @@ async def _communicate_bounded(process: asyncio.subprocess.Process,
688694
while chunk := await process.stdout.read(64 * 1024):
689695
size += len(chunk)
690696
if size > limit:
691-
process.kill()
697+
await edgutil.async_terminate_within(process, _TERMINATE_ALLOWANCE)
692698
return None
693699
chunks.append(chunk)
694-
return b''.join(chunks), await errors
700+
output = b''.join(chunks), await errors
701+
await process.wait()
702+
return output
703+
except BaseException:
704+
await edgutil.async_terminate_within(process, _TERMINATE_ALLOWANCE)
705+
raise
695706
finally:
696707
errors.cancel()
697-
await process.wait()
698708

699709

700710
def basic_auth_header(user: str, password: str) -> str:

‎dev_tools/pylibs/edgutil/__init__.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
55

66
import asyncio
7+
import contextlib
78
import difflib
89
import functools
910
import json
@@ -470,6 +471,24 @@ async def run_task_consumer() -> Any:
470471
asyncio.shield(loop.create_task(run_task_consumer()))
471472
)
472473

474+
async def async_terminate_within(process: asyncio.subprocess.Process,
475+
allowance: float) -> int:
476+
'''Terminate ``process`` (SIGTERM, so it may clean up, e.g. remove its
477+
lock files), killing it should it not exit within ``allowance`` seconds;
478+
return its exit status once it has exited. A process that has already
479+
exited is left alone.
480+
'''
481+
if process.returncode is None:
482+
# Raised should asyncio already be done with it (its transport closed).
483+
with contextlib.suppress(ProcessLookupError):
484+
process.terminate()
485+
try:
486+
await asyncio.wait_for(process.wait(), allowance)
487+
except asyncio.TimeoutError:
488+
with contextlib.suppress(ProcessLookupError):
489+
process.kill()
490+
return await process.wait()
491+
473492
def handle_proc_cancel(proc: Optional[asyncio.subprocess.Process]) -> bool:
474493
'''Terminate the given process prematurely.
475494

‎dev_tools/services/acknowledg-forge/tests/test_safety.py‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import asyncio
99
import io
1010
import os
11+
import signal
1112
import stat
1213
import tarfile
1314
import zipfile
@@ -270,8 +271,29 @@ async def run(*argv: str, limit: int) -> object:
270271

271272
assert await run('sh', '-c', 'printf out; printf err >&2',
272273
limit = 3) == (b'out', b'err')
273-
# One that would never stop is stopped.
274-
assert await run('yes', limit = 1024 * 1024) is None
274+
# One that would never stop is stopped (letting it clean up).
275+
process = await asyncio.create_subprocess_exec(
276+
'yes',
277+
stdout = asyncio.subprocess.PIPE,
278+
stderr = asyncio.subprocess.PIPE
279+
)
280+
assert await _communicate_bounded(process, 1024 * 1024) is None
281+
assert process.returncode == -signal.SIGTERM
282+
283+
284+
async def test_cancelled_git_output_reads_stop_git() -> None:
285+
# As when the service stops while git still has output to write: left
286+
# writing to a pipe no one reads, it would never exit.
287+
process = await asyncio.create_subprocess_exec(
288+
'yes',
289+
stdout = asyncio.subprocess.PIPE,
290+
stderr = asyncio.subprocess.PIPE
291+
)
292+
reading = asyncio.create_task(_communicate_bounded(process, 1 << 40))
293+
await asyncio.sleep(0.2)
294+
reading.cancel()
295+
await asyncio.wait_for(asyncio.gather(reading, return_exceptions = True), 10)
296+
assert process.returncode == -signal.SIGTERM
275297

276298

277299
def test_ci_reads_are_capped(tmp_path: Path,
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM
2+
# Exceptions.
3+
# See https://edgcpp.org/LICENSE.txt for license information.
4+
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
5+
6+
'''Tests for edgutil.async_terminate_within.
7+
8+
Run with: python3 -m unittest discover -s dev_tools/tests
9+
(with dev_tools/pylibs on PYTHONPATH).
10+
'''
11+
12+
import asyncio
13+
import signal
14+
import time
15+
import unittest
16+
17+
import edgutil
18+
19+
class TerminateWithinTests(unittest.IsolatedAsyncioTestCase):
20+
async def test_terminates(self) -> None:
21+
process = await asyncio.create_subprocess_exec('sleep', '30')
22+
started = time.monotonic()
23+
status = await edgutil.async_terminate_within(process, 5)
24+
self.assertEqual(status, -signal.SIGTERM)
25+
self.assertLess(time.monotonic() - started, 2)
26+
27+
async def test_kills_what_outlasts_its_allowance(self) -> None:
28+
# It ignores SIGTERM (as exec keeps ignored signals ignored).
29+
process = await asyncio.create_subprocess_exec(
30+
'sh', '-c', 'trap "" TERM; echo ready; exec sleep 30',
31+
stdout = asyncio.subprocess.PIPE
32+
)
33+
assert process.stdout is not None
34+
self.assertEqual(await process.stdout.readline(), b'ready\n')
35+
started = time.monotonic()
36+
status = await edgutil.async_terminate_within(process, 0.5)
37+
self.assertEqual(status, -signal.SIGKILL)
38+
self.assertGreaterEqual(time.monotonic() - started, 0.5)
39+
40+
async def test_leaves_what_has_exited(self) -> None:
41+
process = await asyncio.create_subprocess_exec('true')
42+
await process.wait()
43+
self.assertEqual(await edgutil.async_terminate_within(process, 5), 0)
44+
45+
if __name__ == '__main__':
46+
unittest.main()

0 commit comments

Comments
 (0)