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.

Sign up for GitHub

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

Jump to bottom

Problem with uvloop and static files #1093

Closed
gleb-chipiga opened this issue Aug 16, 2016 · 18 comments
Closed

Problem with uvloop and static files #1093

gleb-chipiga opened this issue Aug 16, 2016 · 18 comments
Labels
outdated

Comments

@gleb-chipiga
Copy link
Contributor

gleb-chipiga commented Aug 16, 2016 •
edited
Loading

Python/3.5.2 aiohttp/0.22.5 uvloop==0.5.2

Run this code:

import asyncio

import aiohttp.web
import uvloop

asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())

app = aiohttp.web.Application()
app.router.add_static('/static', 'static')
aiohttp.web.run_app(app)

Run the following command:

curl http://127.0.0.1:8080/static/index.html
<html>
    <body>
        some static file
    </body>
</html>HTTP/1.1 200 OK
CONTENT-TYPE: text/html
LAST-MODIFIED: Tue, 16 Aug 2016 23:42:56 GMT
CONTENT-LENGTH: 62
DATE: Tue, 16 Aug 2016 23:43:57 GMT
SERVER: Python/3.5 aiohttp/0.22.5

Headers come after file.
@asvetlov
Copy link
Member

Nice catch!
Funny bug.
I'll take a look.

@asvetlov asvetlov mentioned this issue Aug 25, 2016
Closed
@diogobaeder
Copy link

Affects me as well (as mentioned in the issue referenced above).

asvetlov added a commit that referenced this issue Aug 25, 2016
@asvetlov
Add failed test for #1093
0a3862a
@asvetlov asvetlov mentioned this issue Aug 25, 2016
Merged
@asvetlov
Copy link
Member

Hmmm. Looks like duplicating socket FD removes all guarantees about write ordering.

@1st1 do you have any idea?

#1122 has failed tests for the issue

@1st1
Copy link
Contributor

1st1 commented Aug 25, 2016

Hm. The general idea is to make sure that the headers are flushed, sent, and the original socket is closed. Then do sendfile on a duplicated socket. Not sure how to implement this though.

I also think it's time to add sendfile implementation to asyncio. Will try to work on that.

@asvetlov
Copy link
Member

Closing original socket effectively prevents HTTP pilpelining, which is especially effective for file sending.
As workaround I can disable sendfile for uvloop returning back to read-write loop.
Not the best solution though.

@1st1
Copy link
Contributor

1st1 commented Aug 25, 2016

Does anyone use this in production?

@diogobaeder
Copy link

Not yet, but I'm about to put an aiohttp-based application in production. I kept uvloop out while this issue is being fixed.

@1st1
Copy link
Contributor

1st1 commented Aug 25, 2016

I mean you shouldn't ever serve static files with aiohttp in production -- you should use nginx (or another http server) for that.

@diogobaeder
Copy link

Not in my case, that was going to be used just for a manual test which was going to be run once (or twice) and then disabled.

Your last point is not valid, since a number of applications do serve static files in production as the backend for CDNs. This means that they do provide the original files, although the "big hits" come to the CDN and not the end application. I've also worked with applications serving from Nginx or Apache to a CDN (in both cases it was Akamai), and today I work with an application that serves static files from CherryPy directly to a CDN (Amazon CloudFront). Both styles work well, what matters the most in all of these cases are the quality and features of the CDN.

Anyway, the issue happens in any environment, so it needs a fix either way :-)

@1st1
Copy link
Contributor

1st1 commented Aug 25, 2016

@diogobaeder Sure :) Thank for explaining your use case though.

As workaround I can disable sendfile for uvloop returning back to read-write loop.
Not the best solution though.

@asvetlov Yeah, especially, considering that the old way of doing things (using loop.add_reader on sockets of transports) will be a RuntimeError in the next asyncio release. And, in fact, the same code might not work without uvloop either -- fundamentally this is a race condition. So this has to be fixed.

My solution would be to dup the socket, write headers to the duped socket using socket.send/loop.add_writer APIs, and then use socket.sendfile. The key thing is to send headers and file to the same duped socket, leaving the original transport's socket untouched.

@diogobaeder
Copy link

Thanks, guys, you rock! :-)

@asvetlov
Copy link
Member

@1st1 writing into socket by socket.send/loop.add_writer looks like antipattern. It ends up with reinventing transport but with sendfile support :)
Looks like we really need support sendfile in asyncio itself.

@1st1
Copy link
Contributor

1st1 commented Aug 26, 2016

We'll have the same problem since libuv doesn't support sendfile for transports.

Maybe it is an anti pattern, but you're trying to use syscalls that asyncio does not support and it just happens to work properly.

While writing headers manually will involve some low level coding, it's the only solution that will work now and for months to come.

@nhumrich
Copy link

Just like to put in my $.02 as well. This affects me, and yes I hope to use this in production. I am running an app with gunicorn + aiohttp with uvloop worker right now, I am hoping to add an "admin panel" to the app. Why am I serving with aiohttp instead of nginx/some other server? simple, its an admin panel, not being used by customers, and it makes more sense to deploy it with the backend service.

@asvetlov
Copy link
Member

I'll try to implement @1st1 suggestion for writing headers part without using transport.
Keep in touch.

@1st1
Copy link
Contributor

1st1 commented Aug 29, 2016 via email

asvetlov added a commit that referenced this issue Sep 6, 2016
@asvetlov
Fix sendfile error for uvloop (#1122)
205573b 
* Add failed test for #1093

* Put prepare under implementation methods

* Manually send headers before sendfile call

* Force non-blocking mode for duplicated socket

* Add test for future cancellation in file sender
@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2016

Fixed by #1122

@asvetlov asvetlov closed this as completed Sep 6, 2016
@tomchristie tomchristie mentioned this issue Jul 13, 2018
Closed
@aio-libs-bot aio-libs-bot mentioned this issue Feb 10, 2019
Closed
@lock
Copy link

lock bot commented Oct 29, 2019

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 Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
outdated
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@gleb-chipiga @diogobaeder @1st1 @asvetlov @nhumrich