Multipart can be improved, no security issue in the repository tho.

Okay thank you for the confirmation! I thought as much

I'd love to hear any feedback

Despite claims of improvement,

IMHO. Keep in mind there is a price of complexity, and possibly the addition of other unexpected bugs. Submitting something to a thread pool will pass a lot of logic. Fast is not guaranteed to be efficient.

@nggit thank you for the feedback! The more I've looked into this the more I understand the rats nest I've opened....

I think I'm officially off the threadpool solution since (as @Kludex mentioned) it solves a symptom and not the actual problem.

I hope switching to a faster multi part parser (like multipart) will address most of my concerns with less risk.

Also I'm not sure what you meant by "despite claims of improvement". Feel free to expand on that if I should change something in the parent issue.

With chunks so small, you are of cause paying an enormous overhead with way to many calls to parser.parse() (which impacts python-multipart more than multipart) and are flooding the event loop with tasks that do almost nothing. For a single network packet you are putting thousands of tasks into the event loop, which may explain the poor latency for new clients that try to connect.

I do not think the event loop is stalling on a large task as you assumed earlier. In vanilla Starlette, each write to UploadFile is a context switch and allows other coroutines to run. Even with large chunks (say 64KB) the parsing step should not block for more than a couple of µs with multipart, a little more with python-multipart. But with tiny chunks, the overhead eats up all your CPU time of cause.

That does not sound right. Are you sure?

No! Definitely not, I gathered that from reviewing the code but I didn't test it empirically. Let me run a few tests on various platforms to confirm what I'm seeing.

For a single network packet you are putting thousands of tasks into the event loop, which may explain the poor latency for new clients that try to connect.

This makes a lot more sense then my initial idea! Thank you for the insight; however, let me first confirm the chunk sizes and update this issue.

@defnull you were spot on yet again (dam I really need to start triple checking my work). It seems like my initial test was doing something weird because when I spun up a fresh linux VM it turns out that starlette defaults to using a chunk size of 65536 bytes.

After spending some more time investigating I think I might have found the real issue!! So I went through my vizviewer results with a fine-toothed comb and discovered that the following statement isn't necessarily true

In vanilla Starlette, each write to UploadFile is a context switch and allows other coroutines to run.

Here is the UploadFile code I'm going to be discussing (copied from here)

    @property
    def _in_memory(self) -> bool:
        # check for SpooledTemporaryFile._rolled
        rolled_to_disk = getattr(self.file, "_rolled", True)
        return not rolled_to_disk

    async def write(self, data: bytes) -> None:
        if self.size is not None:
            self.size += len(data)

        if self._in_memory:
            self.file.write(data)
        else:
            await run_in_threadpool(self.file.write, data)

As you can see we only differ to the run_in_threadpool function if the underlying SpooledTemporaryFile has been rolled over. Intuitively this makes sense since before being rolled the file is in-memory and thus it's more efficient to just write directly to the memory buffer.

However, this does not catch the case where the corresponding write causes the file to roll over (like with my 10Mb files). In that case the disk IO happens on the main thread which is very slow and blocks other requests from being processed.

To test my theory I updated the above code to the following which also checks if a write will cause a rollover. With just this change my 100 concurrent runtime test went from Average: 3.02082165s StdDev: 0.87104896832703s to Average: 1.16871194s StdDev: 0.10297384482526314s.

    @property
    def _in_memory(self) -> bool:
        # check for SpooledTemporaryFile._rolled
        rolled_to_disk = getattr(self.file, "_rolled", True)
        return not rolled_to_disk
    
    def _will_roll(self, size_to_add: int)->bool:
        if not self._in_memory:
            return True

        max_size = getattr(self.file, "_max_size", None)
        if max_size is None:
            return False

        return ((self.file.tell()+size_to_add)>self.file._max_size)

    async def write(self, data: bytes) -> None:
        new_data_len = len(data)
        if self.size is not None:
            self.size += new_data_len

        if not self._will_roll(new_data_len):
            self.file.write(data)
        else:
            await run_in_threadpool(self.file.write, data)

I just opened a PR with the proposed change: #2962

However, this does not catch the case where the corresponding write causes the file to roll over (like with my 10Mb files). In that case the disk IO happens on the main thread which is very slow and blocks other requests from being processed.

Wow, great find! That's a serious performance bug (or denail-of-service vulnerability) right there.