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

An error occurred when publishing a static file using uvloop #5149

Closed
hoto17296 opened this issue Oct 26, 2020 · 3 comments · Fixed by #5157
Closed

An error occurred when publishing a static file using uvloop #5149

hoto17296 opened this issue Oct 26, 2020 · 3 comments · Fixed by #5157
Labels
bug need pull request regression Something that used to work stopped working "as before" after upgrade reproducer: present This PR or issue contains code, which reproduce the problem described or clearly understandable STR server

Comments

@hoto17296
Copy link

hoto17296 commented Oct 26, 2020

🐞 Describe the bug
An error occurred when publishing a static file using aiohttp 3.7.x and uvloop.

💡 To Reproduce

# Dockerfile

FROM python:3.8.6

WORKDIR /work

RUN pip install aiohttp==3.7.1 uvloop==0.14.0

ADD app.py .

RUN mkdir static && echo "/* DUMMY */" > static/index.css

CMD ["python", "app.py"]
# app.py

import uvloop
from aiohttp import web

uvloop.install()

app = web.Application()
app.add_routes([web.static("/static", "/work/static")])

if __name__ == "__main__":
    web.run_app(app)

Then, build and run server.

$ docker build . -t tmp
$ docker run --rm -it -p 8080:8080 tmp

Finally, try to access http://localhost:8080/static/index.css .

When accessed from a browser, the following error is displayed in the developer console.

net::ERR_CONTENT_LENGTH_MISMATCH 200 (OK)

When accessed from a cURL, the following error is displayed.

curl: (18) transfer closed with 12 bytes remaining to read

It looks like the connection was closed before sending the response body.

💡 Expected behavior
No error occurs.

📋 Logs/tracebacks
No messages are displayed in the Python log at all.

📋 Your version of the Python

$ docker run --rm tmp python --version       
Python 3.8.6

📋 Your version of the aiohttp/yarl/multidict distributions

$ docker run --rm tmp python -m pip show aiohttp
Name: aiohttp
Version: 3.7.1
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: Nikolay Kim
Author-email: fafhrd91@gmail.com
License: Apache 2
Location: /usr/local/lib/python3.8/site-packages
Requires: chardet, multidict, attrs, async-timeout, typing-extensions, yarl
Required-by:
$ docker run --rm tmp python -m pip show multidict
Name: multidict
Version: 5.0.0
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /usr/local/lib/python3.8/site-packages
Requires: 
Required-by: yarl, aiohttp
$ docker run --rm tmp python -m pip show yarl     
Name: yarl
Version: 1.6.2
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /usr/local/lib/python3.8/site-packages
Requires: idna, multidict
Required-by: aiohttp

📋 Additional context

  • When aiphttp==3.6.*, it works fine
  • When uvloop disabled, it works fine
@hoto17296 hoto17296 added the bug label Oct 26, 2020
@asvetlov
Copy link
Member

Thanks for the very cool reproducible example for the bug!
I wish all bug reports look like this :)

I'm working on an investigation. As a quick workaround you can disable sendfile support for static files by setting AIOHTTP_NOSENDFILE=1 environment variable.

@asvetlov asvetlov added the reproducer: present This PR or issue contains code, which reproduce the problem described or clearly understandable STR label Oct 26, 2020
@asvetlov
Copy link
Member

asvetlov commented Oct 26, 2020

Aha, I see the reason.
uvloop.sendfile() exists but it raises NotImplementedError -- which is 100% correct, it works exactly as I designed asyncio sendfile implementation.
Unfortunately, in aiohttp we forgot to handle this error properly by falling back to a regular loop over file.read() and transport.write().

The thing is not as easy as it should be; earlier aiohttp versions used a hack with sock.dup() and os.sendfile to work around the fact that asyncio did not support native sendfile until Python 3.7. This hack should send the whole message over the copy (dup) of the socket to avoid the corrupted internal state of the asyncio transport.

But the code cannot figure out is the native sendfile supported or not until loop.sendfile() call! Thus, the first transport.write() for sending HTTP headers conflicts with sock.dup() approach, the sent data is broken.

The solution is dropping custom sock.dup() + os.sendfile() workaround for the sake of standard loop.sendfile() and transport.write() fallback. `

loop.sendfile() works pretty well on Python 3.7+, the fallback is called for Python 3.6, compressed data, systems without native os.sendfile (I cannot name them, all Windows, Linux, and MacOS have the native sendfile).

Ironically, uvloop uses the slow fallback until it supports the native loop.sendfile() itself.

This is the plan. Implementation can take a day or two -- depending on my daily job. If somebody wants to pick the issue up and prepare a fix himself -- you are welcome.

@asvetlov asvetlov added backport-3.7 need pull request regression Something that used to work stopped working "as before" after upgrade server labels Oct 26, 2020
@greshilov
Copy link
Contributor

Hey! I've created a PR with fixes, following your plan. I hope I got it right :)

achimnol added a commit to lablup/backend.ai-client-py that referenced this issue Oct 28, 2020
achimnol added a commit to lablup/backend.ai-webserver that referenced this issue Oct 28, 2020
* This fixes an upstream issue (aio-libs/aiohttp#5149) related to
  `sendfile()` fallback with uvloop.
achimnol added a commit to lablup/backend.ai-webserver that referenced this issue Oct 28, 2020
* This fixes an upstream issue (aio-libs/aiohttp#5149) related to
  `sendfile()` fallback with uvloop.
achimnol added a commit to lablup/backend.ai-webserver that referenced this issue Oct 28, 2020
* This fixes an upstream issue (aio-libs/aiohttp#5149) related to
  `sendfile()` fallback with uvloop.

Backported-From: master
Backported-To: 20.03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug need pull request regression Something that used to work stopped working "as before" after upgrade reproducer: present This PR or issue contains code, which reproduce the problem described or clearly understandable STR server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants