openresty
Wait for fd API
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)
fdis the numeric file descriptor to wait oneventsis a string containing "r" and/or "w" indicating to wait for readability vs writabilitytimeoutis the maximum amount of time (in seconds) to wait.
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.
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.
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)
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.
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 :)
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.
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?
@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).
@daurnimator In addition, we may want to make room for the support of waiting on multiple fds at the same time.
@daurnimator Also, please strictly follow the nginx coding style in your patch. For example, 2 blank lines are used to separate function definitions.
@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?
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 Though I admit this PR's goal is meaningful this PR still requires quite some treatment before it can be merged.
@daurnimator as a side note, I greatly appreciate your efforts on this PR. If merged, this PR will drastically change the openresty ecosystem. Personally I've been waiting on zmq abilities for quite awhile. I certainly hope you keep up the good work.
@agentzh Do you have any open notes or comments on this PR to help keep dev moving forward? At this point, the efforts can not even be sponsored because the criteria for PR acceptance is unclear.
@lordnynex I agree. Yeah, I have something in my mind. I'll try drafting an alternative API design for open discussions soon (sorry, I've been too busy with those ngx_stream_* module development and new *_by_lua directives in ngx_lua atm). Thanks for asking.
@daurnimator Not yet, sorry. Been busy with the upcoming OpenResty 1.9.7.1 release.
I talked shortly with @daurnimator on IRC about this pull request. I have been following this, because I have been looking at building some LuaJIT bindings and Lua APIs for things like nanomsg (using it just as an example), and maybe others as well. Right now, to do it in a non-blocking way, you have to kinda start from the scratch (and then have to maintain the lower level network things as well). With this patch it is obvious that I could leverage the existing work done in the official nanomsg project, and I could just write LuaJIT bindings to that, and a nice Lua API on top of it. I think that this way we could grow OpenResty ecosystem to a new heights. And we could also leverage many existing solutions as well – in that sense this adds somewhat a compatibility layer as well (or at least makes it easier).
I remember that on OpenResty Con I was asked about the problems of making OpenResty to talk with ZooKeeper. A quick look at ZooKeeper's C-client shows that this patch could make writing bindings to ZooKeeper quite straightforward.
My knowledge on the lower level things like merging this patch is limited, but I would like to have discussion going on here. I'd like to hear about the concerns @agentzh is having about this, and the ideas for the alternative design. I understand that this is a rather low level library, and maybe somewhat dangerous in wrong hands (?), but it is quite simple as well. Moving it below ngx namespace somewhere like ngx.fd.wait(...) could be a better place for it (?).
Not trying to push or anything, well maybe I am ;-). I agree that we shall get this as correct as possible before releasing. If the API is alright we can possible change the internals later on. But before this is decided, some things cannot be moved forward (like building the actual libraries on top of this).
Edit: We could mark this API as experimental in documentation, and maybe have some compile flag to disable compiling it, although I still would like it to be on by default. Just to get some real world data how this could work (?).
@bungle Thanks for following this discussion.
Basically I'm thinking about a more OO-like API for this. Maybe something like this?
local connection = require "ngx.connection"
local c = connection.new(fd, options)
local ok, err = c:wait_for_read(timeout)
local ok, err = c:wait_for_write(timeout)
local ok, err = c:wait_for_read_write(timeout)
I suggest putting the ngx.connection module in lua-resty-core and uses LuaJIT's FFI to call into the C API (or ABI) provided by the ngx_lua core. In the same spirit of the ngx.semaphore API in development:
https://github.com/openresty/lua-nginx-module/pull/584
https://github.com/openresty/lua-resty-core/pull/5
Just for the reference :)
To be honest, I myself am also eager to have this in OpenResty now :)
One issue: If you try and use this in an init_by_lua context, it fails with a segfault.
Tracking it down: ngx_get_connection calls ngx_drain_connections which tries to dereference ngx_cycle->reusable_connections_queue which is NULL.
The easy fix would be not allowing use of ngx.connection.new in an init context.
But is this an issue that should be fixed elsewhere?
@daurnimator init_by_lua* runs in the nginx master process context which requires some kind of emulation there. The same applies to the existing cosocket API.
@daurnimator init_by_lua runs in the nginx master context which requires some kind of emulation there. The same applies to the existing cosocket API.
Okay; I'll add a context check then :)
It's not working in a timer thread: the call to ngx_http_run_posted_requests fails due to r->connection->data being NULL.
If I add guard the call like:
if (r->connection->data)
ngx_http_run_posted_requests(r->connection);
it works. However I think this is just an insufficient fake request object elsewhere?