Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor pebble multipart parse into a context manager #1440

Open
dimaqq opened this issue Oct 21, 2024 · 2 comments
Open

Refactor pebble multipart parse into a context manager #1440

dimaqq opened this issue Oct 21, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@dimaqq
Copy link
Contributor

dimaqq commented Oct 21, 2024

From: #1437

ruff==0.7.0 flags our use of NamedTemporaryFile, and I believe it is correct to do so.

The temp files are owned by _FilesParser which is used like this:

          parser = _FilesParser(boundary)

          while True:
              chunk = response.read(self._chunk_size)
              if not chunk:
                  break
              parser.feed(chunk)

          resp = parser.get_response()
          if resp is None:
              raise ProtocolError('no "response" field in multipart body')
          self._raise_on_path_error(resp, path)

          filenames = parser.filenames()
          if not filenames:
              raise ProtocolError('no file content in multipart response')
          elif len(filenames) > 1:
              raise ProtocolError('single file request resulted in a multi-file response')

          filename = filenames[0]
          if filename != path:
              raise ProtocolError(f'path not expected: {filename!r}')

          f = parser.get_file(path, encoding)

          parser.remove_files()
          return f

The 2nd raise, if it ever occurs will leave temporary files in the file system.
Given that charm code tends to be rerun on many events, it's likely that the broken code path would be too, leading to accumulation of temporary files until the disk is full or unit is removed.

@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 21, 2024

I've skimmed through our collection of charms and it seems that most common use is:
foo = container.pull(...).read()

With a few variations like foo = reversed(container.pull(...).readlines()).

Perhaps our API doesn't fit the charmers' needs, and we should offer .read_text() / .read_bytes() like pathlib?

@benhoyt
Copy link
Collaborator

benhoyt commented Oct 21, 2024

Yeah, this is a fair point -- thanks for opening an issue.

@benhoyt benhoyt added the bug Something isn't working label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants