Skip to content

[AI] Fix multipart memory limit for buffered files - #1522

Open
puneetdixit200 wants to merge 1 commit into
bottlepy:masterfrom
puneetdixit200:ai-fix-multipart-mem-limit
Open

puneetdixit200 wants to merge 1 commit into
bottlepy:masterfrom
puneetdixit200:ai-fix-multipart-mem-limit

Conversation

@puneetdixit200

Copy link
Copy Markdown

Fixes #1470.

This keeps MEMFILE_MAX as the per-file in-memory threshold for multipart uploads, but stops reusing that same 100 KiB value as the parser-wide total memory limit. The parser now uses its existing larger total-memory default while each individual file part still spools to disk once it exceeds MEMFILE_MAX.

I manually reviewed the diff and verified the regression path with:

  • python3 -m unittest test.test_environ.TestRequest.test_multipart_many_small_files_above_memfile_total
  • python3 -m unittest test.test_environ test.test_multipart
  • python3 -m unittest discover -t . -s test
  • git diff --check HEAD~1..HEAD

AI disclosure: assisted by OpenAI GPT-5. I reviewed the change and test output before submitting.

License: I explicitly license this contribution as public domain.

Allow multipart parsing to keep the existing per-file memory threshold without using that same threshold as the total parser memory limit.

Fixes bottlepy#1470

ai-assisted-by: OpenAI GPT-5

License: Public Domain

@eeshsaxena eeshsaxena left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal (not rejecting two legitimate files whose sizes sum above MEMFILE_MAX) is right, but dropping the mem_limit=self.MEMFILE_MAX argument entirely has a side effect worth a second look.

_MultipartParser.__init__ defaults mem_limit=2 ** 20 and enforces it in the parse loop (if part.size + mem_used > self.mem_limit: raise MultipartError("Memory limit reached.")). So after this change the total in-memory bound for a multipart POST is no longer tied to MEMFILE_MAX at all -- it becomes the hardcoded 1 MB default.

Two consequences:

  1. An app that lowers MEMFILE_MAX (e.g. a memory-constrained service setting it to a few KB) no longer has that reflected in the total buffered-multipart limit; it silently gets 1 MB instead.
  2. memfile_limit (per-part spill-to-disk) and mem_limit (total in-memory) are genuinely different knobs, so coupling both to MEMFILE_MAX was the actual bug -- but the fix leaves mem_limit unconfigurable rather than giving it its own source.

Would it be cleaner to pass an explicit total limit that scales with the configured value, e.g. mem_limit=max(self.MEMFILE_MAX, <sensible floor>), or expose a separate MEMFILE_MAX_TOTAL/mem_limit setting, so the total bound stays under the app`s control instead of reverting to the library default?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New MultipartParser fails on files above 102400 bytes

2 participants