feature: resumed read syscall after interrupted

[spacewander]

No description provided.

[agentzh]

I think we should check n == m inside this do ... while loop as well?

[spacewander]

@agentzh
I think if ferror(fp) != 0 is ture, n should be smaller than m. So there is no need to add another check. If I am wrong, please let me know.

[agentzh]

Alas, seems like readall does not detect or return any errors occurred in the fread() calls. This is bad.

[agentzh]

We should call clearerr() to clear the file error indicator before this line because "the error indicator can be reset only by the clearerr() function" according to the manpage of ferror(). Otherwise we may have an infinite loop here.

[spacewander]

@agentzh
My stupid mistake. I will fix it later.

[thibaultcha]

I too have been intrigued by this. I used this test case to consistently reproduce this behavior:

-- io_intr.lua
local ffi = require "ffi"

ffi.cdef [[
    int getpid(void);
]]

local pid = ffi.C.getpid()

assert(io.popen("kill -17 " .. pid .. " && sleep 0.1"):read("*a"))

Called like so:

$ resty ./io_intr.lua
ERROR: io_intr.lua:36: Interrupted system call
stack traceback:
        io_intr.lua:36: in function 'file_gen'
        init_worker_by_lua:45: in function <init_worker_by_lua:43>
        [C]: in function 'xpcall'
        init_worker_by_lua:52: in function <init_worker_by_lua:50>

I confirm that the proposed patch in this current form produces an infinite loop.

I would like to propose a more concise patch which I believe should work and is closer to the LuaJIT style:

diff --git a/src/lib_io.c b/src/lib_io.c
index 9763ed4..662df42 100644
--- a/src/lib_io.c
+++ b/src/lib_io.c
@@ -164,8 +164,14 @@ static void io_file_readall(lua_State *L, FILE *fp)
   MSize m, n;
   for (m = LUAL_BUFFERSIZE, n = 0; ; m += m) {
     char *buf = lj_buf_tmp(L, m);
-    n += (MSize)fread(buf+n, 1, m-n, fp);
-    if (n != m) {
+    for (;;) {
+      n += (MSize)fread(buf+n, 1, m-n, fp);
+      if (n == m) break;
+      else if (ferror(fp) != 0) {
+        if (errno != EINTR) return;
+        clearerr(fp); /* EINTR, retry */
+        continue;
+      }
       setstrV(L, L->top++, lj_str_new(L, buf, (size_t)n));
       lj_gc_check(L);
       return;

The other benefit of this patch is that upon erroring (non EINTR errors), we will directly return and leave it to io_file_read to catch and throw this error. Before this patch, we still execute setstrV and lj_gc_check before throwing the error.

[spacewander]

@thibaultcha
If the fread success, how does the control flow leave for (;;) loop?

[thibaultcha]

@spacewander I haven't changed the error-free code path from what it was.

[thibaultcha]

Ah, I see what you mean - when we reach the buffer size.

[thibaultcha]

Updated the patch with a simple break which should take care of this case.

[thibaultcha]

In this patch, I was trying to avoid calling fread in multiple places, and dealing with too many repeated conditions (if (m == n)), and, finally, returning early in case of error as described previously. Do you think it is preferable or not?

[spacewander]

@thibaultcha
Yes, your code is more compatible if we want to simply return once error occurs.

[thibaultcha]

(Slightly shortened the above proposed patch.)

[agentzh]

@thibaultcha Will you please create a new PR based on your new patch? It would be great if you can also create a PR for resty-cli to have this covered by its test suite :)

@spacewander asked me privately to ask you to take this job over :)

Many thanks!