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

ngx.socket.tcp receive bsd style #33

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@brg-liuwei

brg-liuwei commented Aug 30, 2016

Implement bsd style receive('*b') for ngx.socket.tcp, and add a simple test case in file t/137-tcp-socket-receive-bsd-style.t

春哥好,今天看到openresty邮件列表里面有关于BSD风格的receive的讨论,我印象中关于这个主题在邮件列表里面已经多次出现了。因此一时兴起写了实现了这个接口,并附上一个简单的测试文件(对Test::Nginx的语法还不完全了解,所以只写一个最简单的test case)。目前还没有去在openresty的文档中添加使用方法,如果这个PR被接受,我再去补充文档,到时候再提交一个PR。英文学得不好,表达不够native,因此前面仅用一段简单的英语做描述,不知道是否符合规范?希望能得到春哥的指点,谢谢。

@agentzh

This comment has been minimized.

Member

agentzh commented Aug 30, 2016

@brg-liuwei Seems like your branch fails the Travis CI testing:

https://travis-ci.org/openresty/stream-lua-nginx-module/builds/156263654

And also see below in this ticket's web UI.

Your branch does not even compile. Will you fix that first?

@mike07026

This comment has been minimized.

mike07026 commented Sep 2, 2016

@brg-liuwei It is useful and it works well, thank you.
Do you mind making another pull request to lua-nginx-module ? I'd like to see cosocket in http supporting bsd style receive.

@brg-liuwei

This comment has been minimized.

brg-liuwei commented Sep 2, 2016

@mike07026 Ok, I'm very glad to make another PR to lua-nginx-module. I will accomplish it soon, please wait a moment.

stream {
server {
listen 5678;
content_by_lua_block {

This comment has been minimized.

@agentzh

agentzh Sep 3, 2016

Member

I think we should make the test as simple as possible to avoid bugs in the test itself. I don't see why the feature requires this complicated is_power_of_two thing in the first place. Will you just output all the packets received? It does not need to be this convoluted IMHO. Thanks!

This comment has been minimized.

@brg-liuwei

brg-liuwei Sep 4, 2016

I just want to mock a text protocol decoding function using is_power_of_two. I think it is better to just output all the packets received. I will modify the test case as your advice.

This comment has been minimized.

@cuiweixie

cuiweixie Sep 4, 2016

I think do something like echo is better. is_power_of_two is just too heavy!

@@ -1990,6 +1995,37 @@ ngx_stream_lua_socket_read_line(void *data, ssize_t bytes)
return NGX_AGAIN;
}

This comment has been minimized.

@agentzh

agentzh Sep 3, 2016

Member

Style: need 2 blank lines to separate C function definitions.

This comment has been minimized.

@brg-liuwei

brg-liuwei Sep 4, 2016

Thanks for your advice

@agentzh

This comment has been minimized.

Member

agentzh commented Sep 3, 2016

@doujiang24 @cuiweixie @yangshuxin Will you have a look at this for me? Many thanks!

@detailyang

This comment has been minimized.

Member

detailyang commented Oct 6, 2016

I have used the api tcpsock:receive('*b') on the repo lua-resty-socks5-server and works well for socks5 server until now:)

@shancci

This comment has been minimized.

shancci commented Nov 26, 2016

when will those PRs to be merged to master? @agentzh @brg-liuwei

@brg-liuwei

This comment has been minimized.

brg-liuwei commented Jan 22, 2017

when will those PRs to be merged to master? @agentzh :)

@agentzh

This comment has been minimized.

Member

agentzh commented Jan 22, 2017

@brg-liuwei I suggest adding this feature to ngx_http_lua_module first since we usually sync changes in only one direction (i.e., from ngx_http_lua to ngx_stream_lua), not both :)

listen 5678;
content_by_lua_block {
local sock = ngx.req.socket(true)
local data, _ = sock:receive('*b')

This comment has been minimized.

@agentzh

agentzh Jan 22, 2017

Member

I suggest we handle and log the errors out and loud, so when tests fail, we can easily see any error messages in the test reports. We should do proper error handling in our own tests.

See other existing tests for examples.

end
data, _ = sock:receive('*b')
if data ~= 'hello world' then

This comment has been minimized.

@agentzh

agentzh Jan 22, 2017

Member

Instead of doing comparison in the Lua code, we prefer doing the comparison in the tests scaffold when comparing the response body to the expected one. You can see other existing test cases for examples.

Please also fix other tests as well.

@@ -1736,6 +1737,10 @@ ngx_stream_lua_socket_tcp_receive(lua_State *L)
u->input_filter = ngx_stream_lua_socket_read_all;
break;
case 'b':

This comment has been minimized.

@agentzh

agentzh Jan 22, 2017

Member

I think c is better here? It means "chunks", just like existing a means "all" and l means "line". "b" is not very descriptive in terms of its semantics.

This comment has been minimized.

@brg-liuwei

brg-liuwei Feb 4, 2017

Hi, agentzh, I tried to modify code as follows:

case 'c': u->input_filter = ngx_http_lua_socket_read_chunks;

but there is a function named ngx_http_lua_socket_read_chunk existing in ngx_http_lua_socket_tcp.c. I'm afraid those names would confuse people who read source code. I hope you give me some good suggestions.

This comment has been minimized.

@agentzh

agentzh Feb 4, 2017

Member

@brg-liuwei Or maybe add a new bsdrecv() method instead? It will also be a little bit faster I think.

sock:settimeout(100)
assert(sock:connect("127.0.0.1", 5678))
sock:send("1")

This comment has been minimized.

@agentzh

agentzh Jan 22, 2017

Member

Please always do error handling so when things go south, we can easily see error messages in the test report.

@detailyang detailyang referenced this pull request Oct 26, 2017

Closed

接受数据超时? #2

@shancci

This comment has been minimized.

shancci commented Oct 28, 2017

Oops! it seems that the #refactor# break all pull codes. @brg-liuwei

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment