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

Work with files asynchronously #3313

Closed
2 tasks done
asvetlov opened this issue Oct 3, 2018 · 12 comments
Closed
2 tasks done

Work with files asynchronously #3313

asvetlov opened this issue Oct 3, 2018 · 12 comments
Labels
enhancement good first issue Good for newcomers Hacktoberfest We think it's good for https://hacktoberfest.digitalocean.com/ outdated

Comments

@asvetlov
Copy link
Member

asvetlov commented Oct 3, 2018

There is a very old tech debt in aiohttp: the library uses blocking synchronous calls to work with files.

It should be fixed by replacement such direct calls by loop.run_in_executor().

Affected parts:

  • web_fileresponse.py
  • payload.py

Splitting the task into two pull requests makes sense in term of easier review.

The issue is pretty easy and straightforward, it waits for a champion.

@aio-libs-bot
Copy link

GitMate.io thinks possibly related issues are #764 (Too many open files), #20 (Refactor files handling.), #1067 (will there be huge file upload support ?), #72 (HTTP POST not working), and #44 (Please document UnixConnection work).

@socketpair
Copy link
Contributor

socketpair commented Oct 3, 2018

Do not forget to open() files also in a thread.

I think, internal API should be the following:

import aiofiles

file = await aiofiles.open()
result = await file.read()
result = await file.write()
await file.chmod()
await file.fsync()
...

UPD: https://github.com/Tinche/aiofiles buth this solution is overingeneered. I would write my own with implementing each method explicitly.

@asvetlov
Copy link
Member Author

asvetlov commented Oct 3, 2018

I don't want to use aiofiles.
run_in_executor is just enough for the library needs.
aiohttp calls open() in file_response.py only, client API accepts already opened regular IO objects.
Wrapping them into aiofiles looks more complex than explicit thread pool usage.

@webknjaz webknjaz added Hacktoberfest We think it's good for https://hacktoberfest.digitalocean.com/ and removed Hacktoberfest We think it's good for https://hacktoberfest.digitalocean.com/ labels Oct 3, 2018
@chirgjn
Copy link

chirgjn commented Nov 30, 2018

👋 would love to pick this up, if not fixed already

@asvetlov
Copy link
Member Author

Please go ahead!

@kxepal
Copy link
Member

kxepal commented Nov 30, 2018

This could raise a holywar, but I'll risk to ask you: why not aiofiles?

@asvetlov
Copy link
Member Author

Did you read messages above?
I can send a ball back: why aiofiles despite the fact that we need only run in executor a couple IO calls?

@kxepal
Copy link
Member

kxepal commented Nov 30, 2018

Yes, I did, but some abstraction may be useful to not care about that bit if it will eventually have changed. If that's the only reason than it satisfied my curiousity (:

@asvetlov
Copy link
Member Author

Changes will not touch the public API.
Choosing an abstraction layer for async files support is the subject for future discussion, let's do it in another issue if you want to.

@asvetlov
Copy link
Member Author

asvetlov commented Jan 3, 2019

Done

@asvetlov asvetlov closed this as completed Jan 3, 2019
@samuelcolvin
Copy link
Member

Woop woop. This is great. Thanks @Tolmachofof

@lock
Copy link

lock bot commented Jan 4, 2020

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Jan 4, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement good first issue Good for newcomers Hacktoberfest We think it's good for https://hacktoberfest.digitalocean.com/ outdated
Projects
None yet
Development

No branches or pull requests

7 participants