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

ngx.socket.tcp receive bsd style #33

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
36 changes: 36 additions & 0 deletions src/ngx_stream_lua_socket_tcp.c
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 @@ -1990,6 +1995,37 @@ ngx_stream_lua_socket_read_line(void *data, ssize_t bytes)
return NGX_AGAIN;
}

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,
Expand Down
76 changes: 76 additions & 0 deletions t/137-tcp-socket-receive-bsd-style.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@

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

repeat_each(10);

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("10")
ngx.sleep(0.01)
sock:send("2")
ngx.sleep(0.01)
sock:send("4")

local pow, _ = sock:receive('*l')
sock:close()
ngx.say(pow)
}
}
--- 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

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 function is_power_of_two(str)
local num = tonumber(str)
if num <= 0 then
return false, nil
end
local power = 0
while num ~= 1 do
if math.fmod(num, 2) ~= 0 then
return false, nil
end
num = num / 2
power = power + 1
end
return true, power
end

local sock = ngx.req.socket(true)
local msg = ''
local pow
while true do
local chunk, err = sock:receive('*b')
if not chunk then
break
end
msg = msg .. chunk
local ok, num = is_power_of_two(msg)
if ok then
pow = num
break
end
end
sock:send(tostring(pow) .. '\n')
}
}
}

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