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

FileResponse works with files asynchronously #3476

Conversation

Tolmachofof
Copy link
Contributor

@Tolmachofof Tolmachofof commented Jan 2, 2019

What do these changes do?

The web FileResponse works with files asynchronously

Are there changes in behavior for the user?

No

Related issue number

#3313

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

filepath.stat() and filepath.open() in def prepare() can block too.
Please run them in an executor as well.

@codecov-io
Copy link

codecov-io commented Jan 2, 2019

Codecov Report

Merging #3476 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3476      +/-   ##
==========================================
+ Coverage   97.93%   97.93%   +<.01%     
==========================================
  Files          44       44              
  Lines        8559     8561       +2     
  Branches     1383     1383              
==========================================
+ Hits         8382     8384       +2     
  Misses         74       74              
  Partials      103      103
Impacted Files Coverage Δ
aiohttp/web_fileresponse.py 96.55% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db8a801...469c311. Read the comment docs.

@asvetlov asvetlov merged commit f81b6a5 into aio-libs:master Jan 2, 2019
@asvetlov
Copy link
Member

asvetlov commented Jan 2, 2019

thanks!

asvetlov pushed a commit that referenced this pull request Jan 2, 2019
* FileResponse works with files asynchronously

* FileResponse: the 'prepare' method works with files asynchronously
@@ -334,8 +337,8 @@ def __init__(self, path: Union[str, pathlib.Path],
self.headers[hdrs.CONTENT_RANGE] = 'bytes {0}-{1}/{2}'.format(
real_start, real_start + count - 1, file_size)

with filepath.open('rb') as fobj:
with (await loop.run_in_executor(None, filepath.open, 'rb')) as fobj:
Copy link
Contributor

Choose a reason for hiding this comment

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

@Tolmachofof blocking close of the fobj

Copy link
Contributor Author

@Tolmachofof Tolmachofof Apr 22, 2019

Choose a reason for hiding this comment

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

@aamalev thanks for your comment!
I think that something like _FileContextManager would solve this problem.
Simplified example:

class _FileContextManager:

    def __init__(self, filepath, flags, start):
        self._filepath = filepath
        self._flags = flags
        self._start = start
        self._fobj = None

    async def __aenter__(self):
        loop = asyncio.get_event_loop()
        self._fobj = await loop.run_in_executor(
            None, self._open, self._filepath, self._start
        )
        return self._fobj

    def _open(self, filepath, start):
        fobj = filepath.open(self._flags)
        if start:
            fobj.seek(start)
        return fobj

    async def __aexit__(self, exc_type, exc_val, exc_tb):
        loop = asyncio.get_event_loop()
        await loop.run_in_executor(None, self._fobj.close)
        self._fobj = None

If this acceptable i can make a PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the conversion with to try..finally is more suitable. _FileContextManager need members loop and executor, and this is too much for a one-time code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. try..finally statement sounds good. I will make a PR.

@lock lock bot added the outdated label Apr 25, 2020
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Apr 25, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants