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

Thread pool is no longer used for files in memory #933

Merged
merged 5 commits into from
May 6, 2020
Merged

Thread pool is no longer used for files in memory #933

merged 5 commits into from
May 6, 2020

Conversation

abersheeran
Copy link
Member

@abersheeran abersheeran commented May 6, 2020

Closes #819

Here I made a judgment, when the file is in memory, no longer use the thread pool to operate. This can save a lot of thread switching time.

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

This is a pretty smart approach yup.

I'm not sure if we want in_memory to be a public API.
Also I guess we could consider changing its check to this?...

rolled_to_disk = getattr(self.file, '_rolled', True)
return not rolled_to_disk

@@ -210,6 +213,17 @@ def test_queryparams():
assert QueryParams(q) == q


@pytest.mark.asyncio
async def test_upload_file():
UploadFile.spool_max_size = 1024
Copy link
Member

Choose a reason for hiding this comment

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

We're changing this class attribute across all the test cases. Probably doesn't really matter, but we could alternately do something like...

TestUploadFile(UploadFile):
    spool_max_size = 1024

And use that?
Also perhaps we should assert that the .read is returning the expected data?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I will modify the test code here.

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Looks good - thanks!

@tomchristie tomchristie merged commit 9725751 into encode:master May 6, 2020
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.

Maybe the thread pool should no longer be used for reading and writing files?
2 participants