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

support sendfile extension in asgi spec #8

Open
synodriver opened this issue Dec 14, 2021 · 24 comments · May be fixed by #20
Open

support sendfile extension in asgi spec #8

synodriver opened this issue Dec 14, 2021 · 24 comments · May be fixed by #20
Labels
enhancement New feature or request

Comments

@synodriver
Copy link

In the current asgi spec, sendfile is supported.
If this webdav server can take advantage of zero-copy-send extension, it would greatly improve server's performance.

当前的ASGI规范中,有个sendfile扩展。如果可以利用这个扩展直接发送文件,那么整个服务器的性能必然可以有很大的提升。

By the way, is there any way to fix that warnings? Iooks like the server send wrong status code.

顺便这有其他可以解决test套件warning的办法吗?看上去就是返回的状态码问题

@rexzhang
Copy link
Owner

sendfile 扩展我找时间研究下。不知你有无这个扩展的使用经验?
test 套件的 warning 是啥情况?能详细描述下么?

@synodriver
Copy link
Author

sendfile 扩展我找时间研究下。不知你有无这个扩展的使用经验? test 套件的 warning 是啥情况?能详细描述下么?

我fork了上游的hypercorn服务器,加入了全部的asgi extension支持,在这
它还支持发送trailerheader,可以当grpc服务器使唤,支持http1~3,由于上游一直不merge,所以我自己发版了

pip install nonecorn

当然,他的命名空间还是hypercorn,目前我将他和gunicorn配合运行了几个月,没发现问题

test 套件的 warning 就是一些兼容性问题,应该是可以修复的

@synodriver
Copy link
Author

对于一个最简单的使用这个扩展的例子,可以看看baize

@synodriver
Copy link
Author

test 套件的 warning 就是一些兼容性问题,应该是可以修复的

比如

cond_put_with_not..... pass
18. cond_put_corrupt_token WARNING: PUT failed with 412 not 423
    ...................... pass (with 1 warning)

@rexzhang
Copy link
Owner

hypercorn 以前尝试过, 当时依赖清单长度惊人,所以很长时间都没关注。刚刚看了下,似乎有所改善。但是对这个库印象不太好。没用过 sendfile 扩展,需要学习下。

@rexzhang
Copy link
Owner

test 套件的 warning 就是一些兼容性问题,应该是可以修复的

比如

cond_put_with_not..... pass
18. cond_put_corrupt_token WARNING: PUT failed with 412 not 423
    ...................... pass (with 1 warning)

这个问题不只是返回状态码的问题,如果要完整实现 RFC 里面的要求,工作量不小,所以这两个告警就一直没解决。有闲心的时候再弄了,毕竟作为 WebDAV 协议实现的标杆 Apache mod_dav 也有这两个告警 ;-p

@synodriver
Copy link
Author

hypercorn 以前尝试过, 当时依赖清单长度惊人,所以很长时间都没关注。刚刚看了下,似乎有所改善。但是对这个库印象不太好。没用过 sendfile 扩展,需要学习下。

hypercorn是所有asgi服务器里面实现最完整的,别的实现目前感觉都缺了东西,有的不支持http/2,理由还一套一套的,有的自称asgi参考服务器,结果几年了,lifespan连影子都没有,所以我就把未来的希望交给了hypercorn,给他加了更多的功能

@synodriver
Copy link
Author

晚上我先来解决zerocopysend的问题吧,毕竟几个asgi服务器的源码都读过了(:

@rexzhang
Copy link
Owner

晚上我先来解决zerocopysend的问题吧,毕竟几个asgi服务器的源码都读过了(:

请务必支持 fallback,确保不支持这个特性的环境能用

@synodriver
Copy link
Author

请务必支持 fallback,确保不支持这个特性的环境能用

这个改动的是底层,http.response.body替换成http.response.zerocopysend,加个判断看看服务器支持与否,不会动上层逻辑,然后是aiofiles打开文件改成os.open,这样才是返回一个int的文件描述符

@rexzhang rexzhang added the enhancement New feature or request label Dec 14, 2021
@synodriver
Copy link
Author

现在发现一点,使用sendfile直接操作文件描述符的时候显然没法做Gzip压缩之类的操作,可能要做些取舍了

@rexzhang
Copy link
Owner

当内容长度低于一个值,当前是 1000bytes,就不会启用Gzip。同时,如果是视频文件之类的已经压缩过的文件,也没有必要启用Gzip

@synodriver
Copy link
Author

目前是如果asgi服务器的sendfile可用的话,那么直接跳过Gzip,直接调用zerocopysend发出去。如果不想要sendfile特性的话,可以用一个asgi中间件取消

class UseSendFileMiddlware:
    def __init__(self, app, open=True):
        self.app = app
        self.open = open

    async def __call__(self, scope,receive,send):
        if not self.open:
            if "extensions" in scope and scope["extensions"].get("http.response.zerocopysend",None):
                scope["extensions"].pop("http.response.zerocopysend")
        await self.app(scope,receive,send)

@rexzhang
Copy link
Owner

gzip 在线压缩还是要支持的

@synodriver
Copy link
Author

gzip 在线压缩还是要支持的

是可以支持的,不过在启用sendfile的情况下无法支持,因为要gzip的话势必读进内存,此时sendfile就没意义了,所以加了个可选的sendfile开关

@rexzhang
Copy link
Owner

gzip 在线压缩还是要支持的

是可以支持的,不过在启用sendfile的情况下无法支持,因为要gzip的话势必读进内存,此时sendfile就没意义了,所以加了个可选的sendfile开关

应该让系统自动去切换;而不是在启动服务的时候定死,这样代价太大,感觉不值。因为磁盘 IO 性能问题,zerocopysend 很多时候发挥不出优势,但是 onfly 压缩对网络环境的适应/优化确是实实在在的

@synodriver
Copy link
Author

应该让系统自动去切换;而不是在启动服务的时候定死,这样代价太大,感觉不值。因为磁盘 IO 性能问题,zerocopysend 很多时候发挥不出优势,但是 onfly 压缩对网络环境的适应/优化确是实实在在的

那改成对小文件启用zerocopysend?大文件走Gzip,小于1000的走zerocopysend?

@rexzhang
Copy link
Owner

要不先放一下,我找时间调整一下代码,让 DAVResponse 处可以知道是否有必要 onfly 压缩,这样就能分别处理

@synodriver
Copy link
Author

要不先放一下,我找时间调整一下代码,让 DAVResponse 处可以知道是否有必要 onfly 压缩,这样就能分别处理

ok,我这逻辑改起来也很方便

@rexzhang
Copy link
Owner

rexzhang commented Jun 16, 2022

5538717 添加了一个新的成员 DAVResponse.compression_method .
由用户基于 content_type 控制响应内容是否压缩,同时添加了这个成员保存压缩的类型. DAVCompressionMethod.NONE 表示未压缩

@rexzhang
Copy link
Owner

未压缩的响应都可以走 zerocopysend, 剩下的问题就是如何告知 ASGI 实现这个相应是否被压缩过了

@synodriver
Copy link
Author

可以写一个中间件来判断,等下我看看

@synodriver
Copy link
Author

synodriver commented Jun 17, 2022

发现个问题,这种typing是cpython3.10才有的新功能,这样写的话3.10以下的都没法用,会出现TypeError: unsupported operand type(s) for |: 'type' and 'NoneType',可以改成Union和从typing模块import的老样式。这次不兼容是在这次提交引入的

@synodriver
Copy link
Author

要改的话,一些asgi相关的typing可以从asgiref里面找,不过这应该是另一个pr了(

@rexzhang rexzhang mentioned this issue Jun 17, 2022
@rexzhang rexzhang linked a pull request Jun 28, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants