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

ngx.socket.tcp receive bsd style #33

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

ngx.socket.tcp receive bsd style #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f628125
ngx.socket.tcp receive bsd style
brg-liuwei Aug 30, 2016
a587647
remove unused variable
brg-liuwei Aug 31, 2016
0b74094
add missing macro: NGX_DEBUG
brg-liuwei Sep 2, 2016
76a1b93
update test case
brg-liuwei Sep 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/ngx_stream_lua_socket_tcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ static int ngx_stream_lua_socket_write_error_retval_handler(
ngx_stream_session_t *s, ngx_stream_lua_socket_tcp_upstream_t *u,
lua_State *L);
static ngx_int_t ngx_stream_lua_socket_read_all(void *data, ssize_t bytes);
static ngx_int_t ngx_stream_lua_socket_read_bsd(void *data, ssize_t bytes);
static ngx_int_t ngx_stream_lua_socket_read_until(void *data, ssize_t bytes);
static ngx_int_t ngx_stream_lua_socket_read_chunk(void *data, ssize_t bytes);
static int ngx_stream_lua_socket_tcp_receiveuntil(lua_State *L);
Expand Down Expand Up @@ -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':
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

u->input_filter = ngx_stream_lua_socket_read_bsd;
break;

default:
return luaL_argerror(L, 2, "bad pattern argument");
break;
Expand Down Expand Up @@ -1991,6 +1996,38 @@ ngx_stream_lua_socket_read_line(void *data, ssize_t bytes)
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your advice


static ngx_int_t
ngx_stream_lua_socket_read_bsd(void *data, ssize_t bytes)
{
ngx_stream_lua_socket_tcp_upstream_t *u = data;

ngx_buf_t *b;

#if (NGX_DEBUG)
ngx_log_debug0(NGX_LOG_DEBUG_STREAM, u->request->connection->log, 0,
"stream lua tcp socket read bsd");
#endif

if (bytes == 0) {
u->ft_type |= NGX_STREAM_LUA_SOCKET_FT_CLOSED;
return NGX_ERROR;
}

b = &u->buffer;

dd("already read: %p: %.*s", u->buf_in,
(int) (u->buf_in->buf->last - u->buf_in->buf->pos),
u->buf_in->buf->pos);

dd("data read: %.*s", (int) bytes, b->pos);

u->buf_in->buf->last = ngx_cpymem(u->buf_in->buf->last, b->pos, bytes);
b->pos += bytes;

return NGX_OK;
}


static ngx_int_t
ngx_stream_lua_socket_tcp_read(ngx_stream_session_t *s,
ngx_stream_lua_socket_tcp_upstream_t *u)
Expand Down
85 changes: 85 additions & 0 deletions t/137-tcp-socket-receive-bsd-style.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
# vim:set ft= ts=4 sw=4 et fdm=marker:

use Test::Nginx::Socket::Lua::Stream 'no_plan';

repeat_each(2);

log_level 'debug';

run_tests();

__DATA__

=== TEST 1: *b pattern for receive
--- config
location = /t {
content_by_lua_block {
local sock = ngx.socket.tcp()
sock:settimeout(100)
assert(sock:connect("127.0.0.1", 5678))

sock:send("1")
Copy link
Member

Choose a reason for hiding this comment

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

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

ngx.sleep(0.01)

sock:send("22")
ngx.sleep(0.01)

local t = {
'test',
'send',
'table',
}
sock:send(t)
ngx.sleep(0.01)

sock:send("hello world")

local data, _ = sock:receive('*a')
ngx.say(data)

sock:close()
}
}
--- main_config
stream {
server {
listen 5678;
content_by_lua_block {
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

@cuiweixie cuiweixie Sep 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

local sock = ngx.req.socket(true)
local data, _ = sock:receive('*b')
Copy link
Member

Choose a reason for hiding this comment

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

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.

if data ~= '1' then
ngx.log(ngx.ERR, "unexcepted result of: ", data)
return
end

data, _ = sock:receive('*b')
if data ~= '22' then
ngx.log(ngx.ERR, "unexcepted result of: ", data)
return
end

data, _ = sock:receive('*b')
if data ~= 'testsendtable' then
ngx.log(ngx.ERR, "unexcepted result of: ", data)
return
end

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

Choose a reason for hiding this comment

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

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.

ngx.log(ngx.ERR, "unexcepted result of: ", data)
return
end

sock:send('ok')
}
}
}

--- request
GET /t
--- response_body
ok
--- no_error_log
[error]
--- error_log
stream lua tcp socket read bsd