feature: add ngx.fs API for async file operation by nginx worker_thread#2489
feature: add ngx.fs API for async file operation by nginx worker_thread#2489oowl wants to merge 3 commits intoopenresty:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new ngx.fs Lua API that performs file operations asynchronously using Nginx worker thread pools (NGX_THREADS), along with an extensive Test::Nginx test suite validating behavior and edge cases.
Changes:
- Introduces
ngx.fs.open()andngx.fs.stat()plus file methods:read(),:write(), and:close()backed by thread-pool tasks. - Injects the new
ngx.fsAPI into thengxglobal when Nginx is built with threads. - Adds a large end-to-end test file covering modes, offsets, errors, concurrency, GC, and abort/kill behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
t/193-async-fs.t |
Adds comprehensive functional/behavioral coverage for ngx.fs APIs. |
src/ngx_http_lua_util.c |
Injects ngx.fs into the ngx Lua API under NGX_THREADS. |
src/ngx_http_lua_async_fs.h |
Declares the ngx.fs injection entrypoint. |
src/ngx_http_lua_async_fs.c |
Implements async open/read/write/stat via thread pools and coroutine resumption. |
config |
Adds new async fs source/header to build lists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case NGX_HTTP_LUA_FS_OP_WRITE: | ||
| if (fs_ctx->err) { | ||
| lua_pushnil(L); | ||
| lua_pushfstring(L, "pwrite() failed (%d: %s)", | ||
| (int) fs_ctx->err, strerror(fs_ctx->err)); |
There was a problem hiding this comment.
file->pool_name is copied from fs_ctx->pool_name without ensuring a terminating NUL byte. Later ngx_http_lua_fs_post_task() formats error messages with "%s" using pool_name->data (which for read/write comes from file->pool_name), so this can read past the end of the buffer and potentially crash or leak memory. Ensure the stored pool name is always NUL-terminated (or avoid %s and format using the explicit length).
| } | ||
|
|
||
| if (offset < 0) { | ||
| return luaL_error(L, "invalid offset: %d", (int) offset); |
There was a problem hiding this comment.
file:read(size, ...) accepts any positive size and casts it to size_t for allocation. This allows unbounded allocations (DoS risk) and can also truncate large lua_Integer values on 32-bit platforms. Consider enforcing a reasonable upper bound and validating that size fits safely in size_t (and update the error formatting to avoid casting to int for messages).
| return luaL_error(L, "invalid offset: %d", (int) offset); | |
| return luaL_error(L, "invalid offset: " LUA_INTEGER_FMT, | |
| (lua_Integer) offset); |
| local before = count_fds() | ||
|
|
||
| local N = 200 | ||
| for i = 1, N do | ||
| local function do_open() | ||
| local f = ngx.fs.open("$TEST_NGINX_HTML_DIR/test.txt", "r") | ||
| if f then | ||
| local _ = f:read(4096) | ||
| f:close() |
There was a problem hiding this comment.
This test’s count_fds() relies on /proc/<pid>/fd and external shell utilities (ls, wc). On systems without /proc (or without these tools), it can silently return an incorrect value, making the fd-leak assertion ineffective and potentially hiding regressions. Consider detecting unsupported environments and skipping/softening this assertion (or using a more portable mechanism) so the test remains meaningful across platforms/CI setups.
I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.
This PR adds asynchronous file I/O support to Lua via a new ngx.fs API built on top of Nginx thread pools.
The implementation follows the same general design as the existing Nginx worker thread API: file operations are dispatched to background threads, the current Lua coroutine yields, and execution resumes in the event loop when the task completes.