Skip to content

Commit

Permalink
Remove tmp_dir arg from AdbCallParser.__init__() + file leak fix.
Browse files Browse the repository at this point in the history
This change removes an unnecessary `tmp_dir` argument from `AdbCallParser`
which already uses `tempfile.NamedTemporaryFile()` which works in all
situations we need. This is important because it simplifies the environment
configuration and what needs to be passed to what during `AndroidEnv`'s
loading, and it'll help to make it more modular.

This change also fixes a bug when installing apps (i.e. `.apk` files) via
`blob` requests where the `.apk` files were allocated and written to, but not
deleted. These files lasted until the (successful) termination of the binary,
and would possibly linger around if terminated abruptly.

PiperOrigin-RevId: 683411795
  • Loading branch information
kenjitoyama authored and copybara-github committed Oct 8, 2024
1 parent 5aa2a46 commit 7a12351
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 234 deletions.
22 changes: 12 additions & 10 deletions android_env/components/adb_call_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,8 @@
class AdbCallParser:
"""Parses AdbRequest messages and executes corresponding adb commands."""

def __init__(self, adb_controller: adb_control.AdbController, tmp_dir: str):
def __init__(self, adb_controller: adb_control.AdbController):
self._adb_controller = adb_controller
self._tmp_dir = tmp_dir
self._handlers = {
'install_apk': self._install_apk,
'start_activity': self._start_activity,
Expand Down Expand Up @@ -276,22 +275,25 @@ def _install_apk(
response.status = adb_pb2.AdbResponse.Status.INTERNAL_ERROR
response.error_message = f'Could not find local_apk_path: {fpath}'
return response

response, _ = self._execute_command(
['install', '-r', '-t', '-g', fpath], timeout=timeout
)
case 'blob':
with tempfile.NamedTemporaryFile(
dir=self._tmp_dir, suffix='.apk', delete=False
) as f:
with tempfile.NamedTemporaryFile(suffix='.apk') as f:
fpath = f.name
f.write(install_apk.blob.contents)

response, _ = self._execute_command(
['install', '-r', '-t', '-g', fpath], timeout=timeout
)
case _:
response.status = adb_pb2.AdbResponse.Status.FAILED_PRECONDITION
response.error_message = (
f'Unsupported `install_apk.location` type: {location_type}'
)
return response

response, _ = self._execute_command(
['install', '-r', '-t', '-g', fpath], timeout=timeout
)
return response

def _start_activity(
Expand Down Expand Up @@ -541,7 +543,7 @@ def _push(
error_message='Push.path is empty.')

# Create temporary file with `push` contents.
with tempfile.NamedTemporaryFile(dir=self._tmp_dir, delete=False) as f:
with tempfile.NamedTemporaryFile(delete=False) as f:
fname = f.name
f.write(request.push.content)
# Issue `adb push` command to upload file.
Expand Down Expand Up @@ -572,7 +574,7 @@ def _pull(
error_message='Pull.path is empty.')

# Issue `adb pull` command to copy it to a temporary file.
with tempfile.NamedTemporaryFile(dir=self._tmp_dir, delete=False) as f:
with tempfile.NamedTemporaryFile(delete=False) as f:
fname = f.name
logging.info('Downloading %r to %r.', path, fname)
response, _ = self._execute_command(['pull', path, fname],
Expand Down
Loading

0 comments on commit 7a12351

Please sign in to comment.