Wait for fd API #450

daurnimator · 2015-01-15T17:45:09Z

Following on from https://groups.google.com/d/msg/openresty-en/T81159uj0y4/ZDckUrpUH30J

The short term goal here is a simple API that allows an advanced user to yield their request until an fd polls ready, or a timeout elapses. An fd polling ready is a primitive used across the majority of unix APIs, and is commonly exposed by (e.g.) database client libraries.

The single function added is: ngx.wait(fd, events, timeout)

fd is the numeric file descriptor to wait on
events is a string containing "r" and/or "w" indicating to wait for readability vs writability
timeout is the maximum amount of time (in seconds) to wait.

agentzh · 2015-02-03T20:50:33Z

it did not address the problem i pointed out earlier :)
that is, it requires unnecessary epoll_ctl syscalls when ET is used.
better make it stateful rather than stateless.

agentzh · 2015-02-03T20:53:12Z

Regarding your offline comments, this is not a premature optimization because system calls are almost always expensive, not to mention the frequency of the wait() calls in reality. Besides, we are designing an API here and this API should make room for important optimizations (even in the future) because once we add an API, we have to maintain backward compatibility.

daurnimator · 2015-02-03T20:53:17Z

it requires unnecessary epoll_ctl syscalls when ET is used.

I feel like that would be premature optimisation.

This is a first step to make a new style of programming possible (i.e. use cqueues).
If it works out well, I hope that a cqueues-like object can provide that stateful api you're talking about
(a cqueues object is essentially an object that contains an epoll fd and a self-pipe to wake itself up)

daurnimator · 2015-02-03T20:56:02Z

this is not a premature optimization because system calls are almost always expensive, not to mention the frequency of the wait() calls in reality. Besides, we are designing an API here and this API should make room for important optimizations (even in the future) because one we add an API, we have to maintain backward compatibility.

If everything works out, this API would be kept for one-shots, where the stateful abstraction would not be helpful. One-shots are quite common, and enough as in a applications an RPC call will be retrieved in a single packet.
A new api that is stateful would be added in parallel.

agentzh · 2015-02-03T20:59:30Z

BTW, I'm against merging quick hacks just to make certain things possible because we have to eventually do it right and we'll almost always pay a bigger price when fixing hacks. Also, I'm very serious about adding new APIs to the ngx_lua core because I'd like to keep the core minimal :)

agentzh · 2015-02-03T21:01:11Z

Having said that, I'd love to see this pull request evolves (here or somewhere else) to a state where it is good to merge.

daurnimator · 2015-02-03T21:36:08Z

BTW, I'm against merging quick hacks just to make certain things possible because we have to eventually do it right and we'll almost always pay a bigger price when fixing hacks.

An extra function useful in some (uncommon) circumstances isn't a quick hack IMO.

Also, I'm very serious about adding new APIs to the ngx_lua core because I'd like to keep the core minimal :)

One of the reasons I would like this to be added in this state is that the functionality provided cannot be done in an external module.
If you could provide a way to have this code outside the core lua-nginx-module I'd be happy to keep things seperate and maintain it myself.
I'd love to start writing+distributing modules that can rely on this type of function.

I'd love to see this pull request evolves

Would you like me to rebase onto master as you work on other things?

a state where it is good to merge.

Could you define what that entails?

agentzh · 2015-02-03T21:43:57Z

Could you define what that entails?

Pass my review :)

agentzh · 2015-02-03T21:46:22Z

@daurnimator Instead of making it a stateless function, I suggest you turn it into something stateful, like the standard Lua io module's file objects. We need at least some kind of (thin) encapsulation here (but no more).

agentzh · 2015-02-03T21:50:25Z

@daurnimator In addition, we may want to make room for the support of waiting on multiple fds at the same time.

agentzh · 2015-02-03T21:52:02Z

@daurnimator Also, please strictly follow the nginx coding style in your patch. For example, 2 blank lines are used to separate function definitions.

agentzh · 2015-02-03T21:53:57Z

src/ngx_http_lua_fd.c

+static void ngx_http_lua_fd_rev_handler(ngx_event_t *ev) {
+    ngx_connection_t *conn = ev->data;
+    ngx_http_lua_udata_t *u = conn->data;
+    ngx_http_request_t *r = u->request;


Please avoid using such complicated initializers becuse it's against the nginx coding style.

Also, please always align up the variables in their definitions.

daurnimator · 2015-02-03T21:57:02Z

@daurnimator Instead of making it a stateless function, I suggest you turn it into something stateful, like the standard Lua io module's file objects.

This seems like a much more complex api that I don't expect to get right on the first go....

What bits of state would you like to keep? the connection object seems to be the only thing possible?
And it comes out of an nginx internal pool anyway....

@daurnimator In addition, we may want to make room for the support of waiting on multiple fds at the same time.

This is the end goal: but I was going to provide it via epoll, kqueue, ports, etc. in a library.
These allow for a single file descriptor to represent/poll many.

@daurnimator Also, please strictly follow the nginx coding style in your patch. For example, 2 blank lines are used to separate function definitions.

I don't suppose there is an 'astyle' incarnation that will make it suit your style?

agentzh · 2015-02-03T21:58:30Z

src/ngx_http_lua_fd.c

+        return luaL_error(L, "no mem");
+    }
+    /* always create a connection object (even if fd is < 0) */
+    if ((u->conn = ngx_get_connection(fd, r->connection->log)) == NULL) {


Calling ngx_get_connection and ngx_free_connection upon every wait() is also too expensive IMHO.

agentzh · 2015-02-03T22:04:17Z

Also, among other things, please add corresponding unit tests to the test suite (but I'm afraid the current API deserves a redesign, so better get the API straight first).

daurnimator · 2017-09-18T10:36:11Z

Hey @agentzh, how can we proceed with an API for waiting for a file descriptor?
Forgive me, but I can't remember the next steps we laid out.

agentzh · 2017-09-20T04:53:01Z

@daurnimator Me either. I need some time to recap. Will look into this as soon as I can manage.

guanlan · 2018-01-31T23:19:21Z

@daurnimator
Here is a good example of testing:
https://github.com/openresty/lua-nginx-module/blob/master/t/077-sleep.t

mergify · 2020-06-26T00:42:08Z