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

Downstream UDP support #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

alonbg
Copy link

@alonbg alonbg commented Sep 8, 2016

Original pull-request was closed as I branched this feature during some repo house keeping.

@alonbg alonbg changed the title Udp support UDP support Sep 8, 2016
@alonbg alonbg changed the title UDP support Downstream UDP support Sep 8, 2016
@bjne
Copy link

bjne commented Oct 25, 2016

&ngx_stream_lua_skcoet_tcp_metatable_key

skcoet -> socket

Conflicts:
	src/ngx_stream_lua_socket_tcp.c
	src/ngx_stream_lua_socket_udp.h
@alonbg
Copy link
Author

alonbg commented Nov 1, 2016

@agentzh, t/099-c-api.t passes on my end but fails on the CI (unable to connect)
Any ideas? Thanks

@agentzh
Copy link
Member

agentzh commented Nov 1, 2016

@alonbg Seems like 099-c-api.t causes the nginx server process to crash. Better use valgrind to locate the memory issues on your side.

@alonbg
Copy link
Author

alonbg commented Nov 1, 2016

export TEST_NGINX_USE_VALGRIND=1; ./go prove -r t/099-c-api.t 1> out.txt 2>&1
@agentzh, sorry :( but I didn't know what to make out of it. attached
out.txt

@doujiang24
Copy link
Member

@agentzh @alonbg
I think the 099-c-api.t failed is because the ngx_http_lua_module changed.
I have created a PR for that #44

And, @agentzh have mark these test cases SKIP, so rebase the lastest master will goes right :)

Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

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

@alonbg Nice job!

{
lua_pushcfunction(L, ngx_stream_lua_tcp_req_socket);
lua_setfield(L, -2, "socket");
}
Copy link
Member

Choose a reason for hiding this comment

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

better keep this function in the same place (before ngx_stream_lua_socket_tcp) that will have a better diff :)

ngx_peer_connection_t *pc;
ngx_stream_lua_srv_conf_t *lscf;
ngx_connection_t *c;
ngx_stream_session_t *s;
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

@@ -3956,6 +3956,11 @@ ngx_stream_lua_req_socket(lua_State *L)

c = s->connection;

if (c->type != SOCK_STREAM) {
return luaL_error(L, "socket api does not match connection transport",
lua_gettop(L));
Copy link
Member

Choose a reason for hiding this comment

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

better remove lua_gettop here?


if (c->type != SOCK_DGRAM) {
return luaL_error(L, "socket api does not match connection transport",
lua_gettop(L));
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

ctx->acquired_raw_req_socket = 1;
#endif

lua_createtable(L, 3 /* narr */, 1 /* nrec */); /* the object */
Copy link
Member

Choose a reason for hiding this comment

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

Seems 2 narr is enough?


ngx_log_debug1(NGX_LOG_DEBUG_STREAM, s->connection->log, 0,
ngx_log_debug1(NGX_LOG_DEBUG_STREAM, c->log, 0,
Copy link
Member

Choose a reason for hiding this comment

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

seems this change not so necessary ?

@@ -138,7 +138,6 @@ lua tcp socket connect timed out
--- timeout: 10



Copy link
Member

Choose a reason for hiding this comment

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

style: three blank lines needed, so we should keep this line :)

u->received = c->buffer->last - c->buffer->pos;
c->buffer->pos =
ngx_copy(ngx_stream_lua_socket_udp_buffer, c->buffer->pos,
u->received);
Copy link
Member

Choose a reason for hiding this comment

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

style: I think this will be better:

        c->buffer->pos = ngx_copy(ngx_stream_lua_socket_udp_buffer,
                                  c->buffer->pos, u->received);

mysock handler aborted
--- no_error_log
[error]
--- wait: 1.1
Copy link
Member

Choose a reason for hiding this comment

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

Seems wait: 0.3 is enough ?

@doujiang24
Copy link
Member

@alonbg I think we'd better create a new directory like t/087-udp-socket and move the udp test files into it.
We'd better keep the same test file number as in lua-nginx-module, what do you think?

@monkeylite
Copy link

When would this PR be merged/released? badly needed:(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants