-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Wait for fd API #450
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
Wait for fd API #450
Changes from all commits
da3738e
ff0240a
5893cd3
112fdd2
14d25dd
b0d214b
6409ee6
e1c1f77
c9d16b4
0bc2837
8b29808
8b39c33
16a4dc7
f07a2d0
c426237
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,353 @@ | ||
/* | ||
* Copyright (C) 2014-2015 Daurnimator | ||
*/ | ||
|
||
/* need to include DDEBUG so that ngx_http_lua_util.h works */ | ||
#ifndef DDEBUG | ||
#define DDEBUG 0 | ||
#endif | ||
#include "ddebug.h" | ||
|
||
#include "ngx_http_lua_connection.h" | ||
|
||
#include <math.h> /* HUGE_VAL */ | ||
#include <poll.h> /* POLLIN, POLLOUT */ | ||
|
||
#include <lua.h> | ||
#include <lauxlib.h> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to include these headers here since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I usually like being explicit here: If you use a declaration in a header; always include that directly (otherwise you get into a mess of implementation specific details). I can remove it if it's against your style though :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator I like shorter code so that we can easily override things in 3rd-party or standard headers (if necessary) in a central place. |
||
|
||
#include "ngx_core.h" | ||
#include "ngx_alloc.h" /* ngx_alloc, ngx_free */ | ||
#include "ngx_http.h" /* ngx_http_request_t, ngx_http_run_posted_requests */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
#include "ngx_http_lua_common.h" /* ngx_http_lua_module, ngx_http_lua_co_ctx_t */ | ||
|
||
#include "ngx_http_lua_util.h" /* ngx_http_lua_get_lua_vm, ngx_http_lua_run_thread, ngx_http_lua_finalize_request */ | ||
#include "ngx_http_lua_contentby.h" /* ngx_http_lua_content_wev_handler */ | ||
|
||
|
||
typedef struct { | ||
ngx_http_request_t *request; | ||
ngx_http_lua_co_ctx_t *co_ctx; | ||
ngx_connection_t *conn; | ||
} ngx_http_lua_udata_t; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
||
|
||
static ngx_int_t | ||
ngx_http_lua_fd_resume_request(ngx_http_request_t *r) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think C function prefixes like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this once missed the rename spree: will rename to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator Ah, I prefer something shorter like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
All the other functions in this file have Aside: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator I'm fine with the file name change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator Lua module names should be a bit more descriptive. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator I think it's fine to use shorter names for different contexts. Huffman's principle are in play :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator For instance, NGINX does not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think I'll rename the file then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator Fine. |
||
{ | ||
ngx_http_lua_ctx_t *ctx; | ||
lua_State *vm; | ||
ngx_int_t rc; | ||
if ((ctx = ngx_http_get_module_ctx(r, ngx_http_lua_module)) == NULL) { | ||
return NGX_ERROR; | ||
} | ||
/* restore normal resume handler */ | ||
ctx->resume_handler = ngx_http_lua_wev_handler; | ||
/* resume lua thread */ | ||
vm = ngx_http_lua_get_lua_vm(r, ctx); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How will you handle repeated event firing when the user code does not completely consume the events under LT (Level-Triggered) mode? You should call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole point of this API is that it will only wait for the event. it's up to the libraries to consume the event. It's cleaned up eventually by calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator The libraries do not always "consume" the event (like only having 2 bytes to write and only reading 3 bytes while 5 bytes are in the recv buffer), in which case, it is our responsibility to de-activate the event for LT events otherwise we're looping here like a hell and there's no way for the "libraries" to manipulate the events directly since its agnostic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator The |
||
lua_pushboolean(ctx->cur_co_ctx->co, 1); | ||
/* resume coroutine */ | ||
rc = ngx_http_lua_run_thread(vm, r, ctx, 1 /*nret*/); | ||
switch (rc) { | ||
case NGX_DONE: /* coroutine finished */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: |
||
ngx_http_lua_finalize_request(r, NGX_DONE); | ||
/* fall-through */ | ||
case NGX_AGAIN: /* coroutine yielded */ | ||
return ngx_http_lua_run_posted_threads(r->connection, vm, r, ctx); | ||
default: /* NGX_ERROR: coroutine failed */ | ||
if (ctx->entered_content_phase) { | ||
ngx_http_lua_finalize_request(r, rc); | ||
return NGX_DONE; | ||
} | ||
return rc; | ||
} | ||
} | ||
|
||
|
||
static void ngx_http_lua_fd_rev_handler(ngx_event_t *ev) { | ||
ngx_connection_t *conn; | ||
ngx_http_lua_udata_t *u; | ||
ngx_http_request_t *r; | ||
ngx_http_lua_co_ctx_t *co_ctx; | ||
ngx_http_lua_ctx_t *ctx; | ||
|
||
conn = ev->data; | ||
u = conn->data; | ||
r = u->request; | ||
co_ctx = u->co_ctx; | ||
|
||
ngx_http_lua_cleanup_pending_operation(co_ctx); | ||
ev = NULL, u = NULL; /* now invalidated */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: we usually avoid such comma expressions in contexts outside the |
||
|
||
if ((ctx = ngx_http_get_module_ctx(r, ngx_http_lua_module)) != NULL) { | ||
/* set current coroutine to the one that had the event */ | ||
ctx->cur_co_ctx = co_ctx; | ||
ctx->resume_handler = ngx_http_lua_fd_resume_request; | ||
/* queue/fire off handler */ | ||
r->write_event_handler(r); | ||
} | ||
} | ||
|
||
|
||
static void | ||
ngx_http_lua_fd_cleanup(ngx_http_lua_co_ctx_t *co_ctx) | ||
{ | ||
ngx_http_lua_udata_t *u = co_ctx->data; | ||
if (u->conn->data) { | ||
/* remove from mainloop; do not pass CLOSE_SOCKET flag */ | ||
ngx_del_conn(u->conn, 0); | ||
u->conn->data = NULL; | ||
} | ||
ngx_free(u); | ||
co_ctx->data = NULL; | ||
} | ||
|
||
|
||
int | ||
ngx_http_lua_connection_init(ngx_connection_t **p, ngx_socket_t fd, const char **err) | ||
{ | ||
ngx_connection_t *conn; | ||
if ((conn = ngx_get_connection(fd, ngx_cycle->log)) == NULL) { | ||
*err = "unable to get nginx connection"; | ||
return NGX_ERROR; | ||
} | ||
conn->data = NULL; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: need a blank line before this line. Please fix other similar places as well. |
||
conn->read->handler = ngx_http_lua_fd_rev_handler; | ||
conn->read->log = conn->log; | ||
conn->write->handler = ngx_http_lua_fd_rev_handler; | ||
conn->write->log = conn->log; | ||
conn->number = ngx_atomic_fetch_add(ngx_connection_counter, 1); | ||
*p = conn; | ||
return NGX_OK; | ||
} | ||
|
||
|
||
void | ||
ngx_http_lua_connection_release(ngx_connection_t *conn) | ||
{ | ||
/* can't use ngx_close_connection here, | ||
as it closes the file descriptor unconditionally */ | ||
|
||
/* cancel timeout timer */ | ||
if (conn->read->timer_set) { | ||
ngx_del_timer(conn->read); | ||
} | ||
|
||
if (conn->data) { | ||
/* remove from mainloop; do not pass CLOSE_SOCKET flag */ | ||
ngx_del_conn(conn, 0); | ||
} | ||
|
||
/* delete any pending but not handled events */ | ||
#if defined(nginx_version) && nginx_version >= 1007005 | ||
if (conn->read->posted) { | ||
ngx_delete_posted_event(conn->read); | ||
} | ||
if (conn->write->posted) { | ||
ngx_delete_posted_event(conn->write); | ||
} | ||
#else | ||
if (conn->read->prev) { | ||
ngx_delete_posted_event(conn->read); | ||
} | ||
if (conn->write->prev) { | ||
ngx_delete_posted_event(conn->write); | ||
} | ||
#endif | ||
|
||
/* mark as non-reusable */ | ||
ngx_reusable_connection(conn, 0); | ||
|
||
/* invalidate connection object */ | ||
conn->fd = -1; | ||
conn->data = NULL; | ||
ngx_free_connection(conn); | ||
} | ||
|
||
|
||
int | ||
ngx_http_lua_connection_prep(ngx_http_request_t *r, ngx_connection_t *conn, | ||
int poll_mask, ngx_msec_t wait_ms, const char **err) | ||
{ | ||
ngx_http_lua_ctx_t *ctx; | ||
ngx_http_lua_co_ctx_t *co_ctx; | ||
ngx_http_lua_udata_t *u; | ||
if ((ctx = ngx_http_get_module_ctx(r, ngx_http_lua_module)) == NULL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: we need a blank line before this line. Please also fix other similar places. |
||
*err ="no request ctx found"; | ||
return NGX_ERROR; | ||
} | ||
if ((co_ctx = ctx->cur_co_ctx) == NULL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: we need a blank line before this line. Please fix other similar places. |
||
*err ="no co ctx found"; | ||
return NGX_ERROR; | ||
} | ||
if ((u = ngx_alloc(sizeof(*u), r->connection->log)) == NULL) { | ||
*err ="no memory"; | ||
return NGX_ERROR; | ||
} | ||
/* cleanup old events before adding new ones */ | ||
ngx_http_lua_cleanup_pending_operation(co_ctx); | ||
if ((poll_mask & POLLIN) && ngx_add_event(conn->read, NGX_READ_EVENT, NGX_LEVEL_EVENT) != NGX_OK) { | ||
ngx_free(u); | ||
*err ="unable to add to nginx main loop"; | ||
return NGX_ERROR; | ||
} | ||
if ((poll_mask & POLLOUT) && ngx_add_event(conn->write, NGX_WRITE_EVENT, NGX_LEVEL_EVENT) != NGX_OK) { | ||
if (poll_mask & POLLIN) { | ||
ngx_del_event(conn->read, NGX_READ_EVENT, 0); | ||
} | ||
ngx_free(u); | ||
*err ="unable to add to nginx main loop"; | ||
return NGX_ERROR; | ||
} | ||
conn->data = u; | ||
u->request = r; | ||
u->co_ctx = co_ctx; | ||
u->conn = conn; | ||
co_ctx->cleanup = (ngx_http_cleanup_pt)&ngx_http_lua_fd_cleanup; | ||
co_ctx->data = u; | ||
|
||
if (wait_ms != (ngx_msec_t)-1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: need a space after the type cast expression |
||
ngx_add_timer(conn->read, wait_ms); | ||
} | ||
|
||
/* make sure nginx knows what to do next */ | ||
if (ctx->entered_content_phase) { | ||
r->write_event_handler = ngx_http_lua_content_wev_handler; | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: need a blank line before this |
||
r->write_event_handler = ngx_http_core_run_phases; | ||
} | ||
|
||
return NGX_OK; | ||
} | ||
|
||
|
||
/* Lua C API bindings */ | ||
|
||
static int | ||
ngx_http_lua_connection_new(lua_State *L) | ||
{ | ||
ngx_connection_t **pconn; | ||
ngx_socket_t fd = luaL_checkinteger(L, 1); | ||
const char *errmsg; | ||
luaL_argcheck(L, fd >= 0, 1, "invalid file descriptor"); | ||
pconn = lua_newuserdata(L, sizeof(*pconn)); | ||
*pconn = NULL; | ||
luaL_getmetatable(L, NGX_HTTP_LUA_CONNECTION_KEY); | ||
lua_setmetatable(L, -2); | ||
if (NGX_ERROR == ngx_http_lua_connection_init(pconn, fd, &errmsg)) { | ||
return luaL_error(L, errmsg); | ||
} | ||
(*pconn)->data = NULL; | ||
return 1; | ||
} | ||
|
||
|
||
static ngx_connection_t* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: need a space before |
||
ngx_http_lua_connection_check(lua_State *L, int idx) | ||
{ | ||
ngx_connection_t **pconn = luaL_checkudata(L, idx, NGX_HTTP_LUA_CONNECTION_KEY); | ||
luaL_argcheck(L, *pconn != NULL, idx, "ngx_connection_t has been freed"); | ||
return *pconn; | ||
} | ||
|
||
|
||
static int | ||
ngx_http_lua_connection_gc(lua_State *L) | ||
{ | ||
ngx_connection_t **pconn = luaL_checkudata(L, 1, NGX_HTTP_LUA_CONNECTION_KEY); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should avoid such C99 feature that can upset older compilers. Please fix similar places too. Thank you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the C99 feature used here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator Sorry, it's not really a C standard thing. It's really an NGINX coding style thing. We should use constant expressions in variable initializations. |
||
if (NULL != *pconn) { | ||
ngx_http_lua_connection_release(*pconn); | ||
*pconn = NULL; | ||
} | ||
return 0; | ||
} | ||
|
||
|
||
static int | ||
ngx_http_lua_connection_tostring(lua_State *L) | ||
{ | ||
ngx_connection_t *conn = ngx_http_lua_connection_check(L, 1); | ||
lua_pushfstring(L, "ngx_connection*: %p", conn); | ||
return 1; | ||
} | ||
|
||
|
||
static int | ||
ngx_http_lua_connection_wait(lua_State *L) | ||
{ | ||
ngx_connection_t *conn = ngx_http_lua_connection_check(L, 1); | ||
int poll_mask; | ||
double timeout; | ||
const char *events, *err; | ||
ngx_http_request_t *r; | ||
|
||
switch(lua_type(L, 2)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Such tests are a bit wasteful. I suggest we expose specialized |
||
case LUA_TSTRING: | ||
events = lua_tostring(L, 2); | ||
poll_mask = 0; | ||
while (*events) { | ||
if (*events == 'r') { | ||
poll_mask |= POLLIN; | ||
} else if (*events == 'w') { | ||
poll_mask |= POLLOUT; | ||
} | ||
events++; | ||
} | ||
break; | ||
case LUA_TNUMBER: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: need a blank line before each |
||
poll_mask = lua_tointeger(L, 2); | ||
if (!(poll_mask & ~(POLLIN|POLLOUT))) { | ||
break; | ||
} | ||
/* fall through on invalid poll_mask */ | ||
default: | ||
return luaL_argerror(L, 2, "expected bitwise 'or' of POLLIN|POLLOUT or characters from set 'rw'"); | ||
} | ||
|
||
timeout = luaL_optnumber(L, 3, HUGE_VAL); /* default to infinite timeout */ | ||
if (!poll_mask && timeout == HUGE_VAL) { | ||
return luaL_error(L, "must provide a valid events mask or finite timeout"); | ||
} | ||
if ((r = ngx_http_lua_get_req(L)) == NULL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: please avoid using assignments in |
||
return luaL_error(L, "no request found"); | ||
} | ||
|
||
if (NGX_ERROR == ngx_http_lua_connection_prep(r, conn, poll_mask, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: please put the constant on the right hand side of comparison operators. (I do understand the rationale before this style but 1) modern C compilers already emit warnings for such misuses and 2) we should follow nginx's coding style). |
||
(timeout < 0)? 0 : (timeout == HUGE_VAL)? ((ngx_msec_t)-1) : timeout*1000, &err)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: |
||
return luaL_error(L, err); | ||
} | ||
|
||
return lua_yield(L, 0); | ||
} | ||
|
||
|
||
void | ||
ngx_http_lua_inject_connection(lua_State *L) | ||
{ | ||
luaL_newmetatable(L, NGX_HTTP_LUA_CONNECTION_KEY); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest hooking the metatable onto the local ngx_connection = require "ngx.connection"
local wait_read = ngx_connection.wait_read
...
-- on hot code paths:
local ok, err = wait_read(c) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It already is... see the following ~10 lines. |
||
lua_pushcfunction(L, ngx_http_lua_connection_gc); | ||
lua_setfield(L, -2, "__gc"); | ||
lua_pushcfunction(L, ngx_http_lua_connection_tostring); | ||
lua_setfield(L, -2, "__tostring"); | ||
lua_newtable(L); | ||
lua_pushcfunction(L, ngx_http_lua_connection_wait); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lua_CFunction cannot be JIT compiled. I suggest we expose pure C API in the ngx_lua core and do the Lua wrapper API via FFI in lua-resty-core instead. I don't want to support the standard Lua interpreter for these new APIs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please take a look at how the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have made the functions available for ffi. Notice all the functions that were not declared There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator I don't want to keep compatibility with the standard Lua interpreter for new nontrivial APIs now since it's quite a maintenance burden. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need them myself.... e.g. I debug using standard lua; then once it works there, I go debug with luajit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator That's a reasonable approach but unfortunately we are accumulating features exclusive for LuaJIT. |
||
lua_setfield(L, -2, "wait"); | ||
lua_pushcfunction(L, ngx_http_lua_connection_gc); | ||
lua_setfield(L, -2, "free"); | ||
lua_setfield(L, -2, "__index"); | ||
lua_pop(L, 1); | ||
|
||
lua_newtable(L); | ||
lua_pushcfunction(L, ngx_http_lua_connection_new); | ||
lua_setfield(L, -2, "new"); | ||
lua_pushcfunction(L, ngx_http_lua_connection_wait); | ||
lua_setfield(L, -2, "wait"); | ||
lua_pushcfunction(L, ngx_http_lua_connection_gc); | ||
lua_setfield(L, -2, "free"); | ||
lua_pushinteger(L, POLLIN); | ||
lua_setfield(L, -2, "POLLIN"); | ||
lua_pushinteger(L, POLLOUT); | ||
lua_setfield(L, -2, "POLLOUT"); | ||
|
||
lua_setfield(L, -2, "connection"); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Copyright (C) 2014 Daurnimator | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daurnimator Will you please state that "I hereby assign copyright in this code to the lua-nginx-module project, to be licensed under the same terms as the rest of the code"? This is just for legal resolution. Thanks for your understanding. |
||
*/ | ||
|
||
#ifndef _NGX_HTTP_LUA_CONNECTION_H_INCLUDED_ | ||
#define _NGX_HTTP_LUA_CONNECTION_H_INCLUDED_ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: need 2 blank lines here. Please fix other similar places. Thank you. |
||
#include <lua.h> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, no need to explicitly include |
||
#include "ngx_http_lua_common.h" | ||
|
||
#define NGX_HTTP_LUA_CONNECTION_KEY "ngx_connection_t*" | ||
|
||
int ngx_http_lua_connection_init(ngx_connection_t **p, ngx_socket_t fd, const char **err); | ||
void ngx_http_lua_connection_release(ngx_connection_t *conn); | ||
int ngx_http_lua_connection_prep(ngx_http_request_t *r, ngx_connection_t *conn, | ||
int poll_mask, ngx_msec_t wait_ms, const char **err); | ||
void ngx_http_lua_inject_connection(lua_State *L); | ||
|
||
|
||
#endif /* _NGX_HTTP_LUA_CONNECTION_H_INCLUDED_ */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't rely on poll since the underlying events type can be arbitrary. Better use our own constants or borrow nginx's
NGX_READ_EVENT
andNGX_WRITE_EVENT
for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most libraries return values POLLIN/POLLOUT. This is for convenience so that you don't need to transfrom from OS standard constants to nginx specific ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daurnimator Use of these constants make the reader think we are actually using poll(). Also please keep in mind we need to support non-POSIX systems where no poll() exists, like Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a poll on windows. and it uses the same constants: https://msdn.microsoft.com/en-us/library/windows/desktop/ms740094(v=vs.85).aspx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daurnimator Are you sure? There's not even a
poll.h
there on Windows :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daurnimator I think you've already got my points so I won't repeat them here :)
You code won't even compile on Windows as-is BTW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I just looked it up, and NGX_READ_EVENT is a #define to POLLIN on linux anyway.
The problem is that NGX_READ_EVENT is not safe to be used as a bitmask.
e.g. see the
select
#ifdef
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daurnimator That's why NGINX introduced such indirections in the first place so that nginx's build system can point them to the right things on different platforms :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like in this case I'll need to invent my own identifiers then? (because there is no
NGX_NO_EVENT
orNGX_READ_WRITE_EVENT
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daurnimator Yes, that sounds better to me.