udp downstream api - work in progress #28
Conversation
| ngx_peer_connection_t *pc; | ||
| int n, raw; | ||
| ngx_stream_session_t *s; | ||
| ngx_peer_connection_t *pc; |
There was a problem hiding this comment.
Style: indentation problems here?
There was a problem hiding this comment.
Please fix the indentation issues in the rest of your changes. We use 4-space indentation consistently.
|
Will you add some test cases to the existing test suite? Also, please rebase to the current git master branch in your own branch. Many thanks! |
| @@ -83,7 +90,7 @@ ngx_stream_lua_inject_socket_udp_api(ngx_log_t *log, lua_State *L) | |||
| lua_setfield(L, -2, "udp"); /* ngx socket */ | |||
|
|
|||
| /* udp socket object metatable */ | |||
| lua_pushlightuserdata(L, &ngx_stream_lua_socket_udp_metatable_key); | |||
| lua_pushlightuserdata(L, &ngx_stream_lua_udp_socket_metatable_key); | |||
There was a problem hiding this comment.
This name change is confusing. We regard ngx_stream_lua_socket_ as something like a namespace.
There was a problem hiding this comment.
it matchs tcp.
I can change both :)
There was a problem hiding this comment.
Not sure Test::Nginx has proper support for udp.
new issue under test-nginx
There was a problem hiding this comment.
@alonbg Yeah, we need to add UDP support to that test class :) Patches welcome! :)
|
@agentzh |
| if (c->type != SOCK_DGRAM) { | ||
| return luaL_error(L, "socket api does not match connection transport", | ||
| lua_gettop(L)); | ||
| } |
|
all comments committed. rebased. |
|
@agentzh tests still fail. Please advise :) |
|
@alonbg The "Connection refused" error reported by the test scaffold is a clear indicator of nginx server crashes (segmentation faults). In this case, we should run that offending test block individually with the valgrind test mode to pinpoint the memory issue in the code base. |
|
@alonbg I'm also getting valgrind memory errors while running your tests on my side. For example, |
|
@alonbg BTW, please rebase to the latest git master for some travis ci fixes (due to recent travis ci server environment changes. alas). |
|
@agentzh |
|
@alonbg I think you encountered this while working on ngx_stream_echo_module as well. You cannot use |
|
@agentzh yes I remembered that :) |
|
@alonbg What API should you use? Sorry, I don't know what kind of refactoring you are going to do. I only have the time to comment on your code changes atm. Please see the existing code for a better idea of how to use the nginx C API (and also those nginx programming tutorials out there on the web). |
|
@alonbg I suggest you skip those |
|
Thanks @agentzh . Going through those tutorials as we speak :). As you suggested, I'll probably skip these functions. If so, I make sure to error handle calls in udp context. |
|
@alonbg Yeah, throwing out a Lua exception in those unsupported Lua APIs is important. |
|
@alonbg Please rebase to the latest master which contains the cjson travis ci fix. |
| end | ||
|
|
||
| for i = 1, 3 do | ||
| local data, err, part = sock:receive(5) |
There was a problem hiding this comment.
The receive() method should not take any argument. Datagrams are atomic, unlike streams.
|
@alonbg I've just made a dirty and quick patch against your branch that makes the simplest test case pass completely on my side. Just for your reference: diff --git a/src/ngx_stream_lua_socket_udp.c b/src/ngx_stream_lua_socket_udp.c
index d066685..366f7a4 100644
--- a/src/ngx_stream_lua_socket_udp.c
+++ b/src/ngx_stream_lua_socket_udp.c
@@ -1058,7 +1058,7 @@ ngx_stream_lua_socket_udp_send(lua_State *L)
dd("sending query %.*s", (int) query.len, query.data);
- n = ngx_send(u->udp_connection.connection, query.data, query.len);
+ n = ngx_udp_send(u->udp_connection.connection, query.data, query.len);
dd("ngx_send returns %d (query len %d)", (int) n, (int) query.len);
@@ -1087,6 +1087,7 @@ ngx_stream_lua_socket_udp_receive(lua_State *L)
{
ngx_stream_session_t *s;
ngx_stream_lua_socket_udp_upstream_t *u;
+ ngx_connection_t *c;
ngx_int_t rc;
ngx_stream_lua_ctx_t *ctx;
ngx_stream_lua_co_ctx_t *coctx;
@@ -1156,7 +1157,19 @@ ngx_stream_lua_socket_udp_receive(lua_State *L)
"stream lua udp socket receive buffer size: %uz",
u->recv_buf_size);
- rc = ngx_stream_lua_socket_udp_read(s, u);
+ c = u->udp_connection.connection;
+
+ if (u->raw_downstream && !u->connected) {
+ u->received = c->buffer->last - c->buffer->pos;
+ c->buffer->pos =
+ ngx_copy(ngx_stream_lua_socket_udp_buffer, c->buffer->pos, u->received);
+ ngx_stream_lua_socket_udp_handle_success(s, u);
+ u->connected = 1;
+ rc = NGX_OK;
+
+ } else {
+ rc = ngx_stream_lua_socket_udp_read(s, u);
+ }
if (rc == NGX_ERROR) {
dd("read failed: %d", (int) u->ft_type);
@@ -1268,7 +1281,7 @@ ngx_stream_lua_socket_udp_finalize(ngx_stream_session_t *s,
u->resolved->ctx = NULL;
}
- if (u->udp_connection.connection) {
+ if (u->udp_connection.connection && !u->raw_downstream) {
ngx_log_debug0(NGX_LOG_DEBUG_STREAM, s->connection->log, 0,
"stream lua close socket connection");
diff --git a/src/ngx_stream_lua_socket_udp.h b/src/ngx_stream_lua_socket_udp.h
index b147b54..e7ca378 100644
--- a/src/ngx_stream_lua_socket_udp.h
+++ b/src/ngx_stream_lua_socket_udp.h
@@ -53,7 +53,8 @@ struct ngx_stream_lua_socket_udp_upstream_s {
ngx_stream_lua_co_ctx_t *co_ctx;
unsigned raw_downstream:1;
- unsigned waiting; /* :1 */
+ unsigned waiting:1;
+ unsigned connected:1;
};
diff --git a/t/138-req-udp-socket.t b/t/138-req-udp-socket.t
index f143c6e..f2b70fb 100644
--- a/t/138-req-udp-socket.t
+++ b/t/138-req-udp-socket.t
@@ -20,31 +20,33 @@ run_tests();
__DATA__
=== TEST 1: sanity
+--- ONLY
--- dgram_server_config
content_by_lua_block {
local sock, err = ngx.req.udp_socket()
- if sock then
- ngx.say("got the request socket")
- else
- ngx.say("failed to get the request socket: ", err)
+ if not sock then
+ ngx.log(ngx.ERR, "failed to get the request socket: ", err)
+ return ngx.exit(ngx.ERROR)
end
- for i = 1, 3 do
- local data, err, part = sock:receive(5)
- if data then
- ngx.say("received: ", data)
- else
- ngx.say("failed to receive: ", err, " [", part, "]")
- end
+ local data, err = sock:receive()
+ if not data then
+ ngx.log(ngx.ERR, "failed to receive: ", err)
+ return ngx.exit(ngx.ERROR)
+ end
+
+ print("data: ", data)
+
+ local ok, err = sock:send("received: " .. data)
+ if not ok then
+ ngx.log(ngx.ERR, "failed to send: ", err)
+ return ngx.exit(ngx.ERROR)
end
}
--- dgram_request chomp
hello world! my
---- dgram_response
-got the request socket
-received: hello
-received: worl
-received: d! my
+--- dgram_response chomp
+received: hello world! my
--- no_error_log
[error]
|
|
What is the status of this PR, is there any assistance needed? I came across a need for this capability in one of my projects. |
|
@splitice With my dirty patch above, it is good enough for my DNS name server project already. Though it's not good enough for merging into master :) |
|
Cool, And FYI - my project is to implement a caching DNS Proxy. Seems like I am seeing a pattern out of everyone who wants this patch. |
|
FYI: https://twitter.com/agentzh/status/766062354928955392 For the best result there, I had to enable the trace stitching feature of LuaJIT 2.1 (via |
|
Anyway, I'm gonna deploy my pure Lua DNS nameserver atop this NGINX module to power my own openresty.org domain very soon :) |
|
@agentzh Damn, thats fast. I'm hoping to get at-least that. My ideal target is ~200 - 400K r/s with 4 - 8 workers (assuming high cache hit rate, and some fairly beefy CPUs). Any chance you plan to opensource your server? It would be very interesting to see how you have architected it vs what I have done so far. My needs dictate a proxy, rather than a server per-say but there is no reason if open sourced not to extend / re-use if possible, |
|
@splitice Just created a gist containing everything needed to run my Lua DNS server atop NGINX yourself: https://gist.github.com/agentzh/6c50d37510daef792ed220fa0d970393 It's very hacky though ;) There is still room for optimizations. See the C-land on-CPU flamegraph while my Lua DNS server atop NGINX is loaded by dnsperf: http://agentzh.org/misc/flamegraph/lua-dns-server-on-nginx-2016-08-17.svg Basically with a pure C DNS parser and generator can call them via FFI should be faster. Also, we could recycle the Lua thread object for each coroutine, reducing GC overhead. Have fun :) |
|
@agentzh absolutely fabulous For the upstream manipulation I thought of making this And for the downstream I have no direction yet. An alternative to the above would be to implement the Any kind of help you can offer would be great |
|
@alonbg You may find this PR for ngx_http_lua_module interesting: openresty/lua-nginx-module#712 It adds the |
|
@alonbg I don't think I'm working under the assumption that (unfortunately) the full manipulation has to occur in the @agentzh thanks for that, I'll read over it over the weekend. |
|
@agentzh I'll look into the bind PR in ngx_http_lua_module. Thank you. @splitice That's a very important confirmation. Thank you for sharing your thoughts :). I believe I'll continue working under those same assumptions. |
| ngx_stream_lua_socket_udp_handle_success(s, u); | ||
| u->connected = 1; | ||
| rc = NGX_OK; | ||
| } else { |
There was a problem hiding this comment.
Style: need a blank line before } else {. Please fix other places as well. Thank you.
|
@agentzh this awaits your approval or is there anything else todo on this pull request? |
|
@agentzh Can this be useful for your dns impl: |
|
continues in a new pull request |
nginx.conf with both tcp and udp test servers
testing udp: not working ( just yet ... )
client:
socat -T10 -t10 -d -d -d - udp:127.0.0.1:8070echo server:
socat -d -d -d udp-l:8071,reuseaddr,fork,crlf system:"cat"testing tcp: working just fine
client:
socat -d -d -d - tcp:127.0.0.1:5354echo server: `socat -d -d -d tcp-l:5355,reuseaddr,fork,crlf system:"cat"