bugfix: io module: ensured read() resumes read in all modes upon EINTR. #17
Conversation
src/lib_io.c
Outdated
| { | ||
| MSize m = LUAL_BUFFERSIZE, n = 0, ok = 0; | ||
| char *buf; | ||
| MSize m = LUAL_BUFFERSIZE, n = 0, ok = 0, s = 0; |
There was a problem hiding this comment.
I don't think using the name s for size values is a good idea. It looks like string :)
There was a problem hiding this comment.
Updated the name to size
|
I don't spot logic error, but the code layout can be improved. IIRC, in some static compilers, when it compiles if-then-else construct, if there is no annotation indicating which target is more frequent, it will assume then-clause is more frequent. This PR just make static compiler make wrong decision. Maybe you can use LJ_UNLIKELY to improve the code layout. |
src/lib_io.c
Outdated
| if (errno == EINTR) { clearerr(fp); continue; } | ||
| return 0; | ||
| } else { | ||
| setnilV(L->top++); |
There was a problem hiding this comment.
Maybe it is no need to set the L->top to nil, as the L->top will be set to nil later if the return value is zero.
https://github.com/openresty/luajit2/blob/v2.1-agentzh/src/lib_io.c#L230
Remember to increase L->top by one once the return value is zero. Another option is to remove the if (!ok) check and set L->top to nil immediately in all branches.
There was a problem hiding this comment.
Good catch - I try to minimize the impact of this patch on the upstream source as much as possible. The upstream patch (see LuaJIT/LuaJIT#376) has other improvements to the IO module, but this patch (against the OpenResty fork) focuses solely on EINTR errors. I do not think it is its role to fix other types of behavior if it means the patch size will increase more than needed.
I'll definitely add this to the upstream patch though!
There was a problem hiding this comment.
(That said, this is my vision of the patch but I am pretty flexible about it, if it is deemed best :) )
|
@yangshuxin In most (all?) cases in this PR, the error branch comes first, and the error-free branch comes second (then-clause). If what you say is true, I am not sure which branches you are referring to? |
thibaultcha
force-pushed
the
fix/eintr-bis
branch
from
6cdd820 to
f980786
Compare
|
@yangshuxin Branch prediction builtin added, thanks for the suggestion. I added them with a rule of thumb to only try to optimize the error-free branch of each function. |
|
We now both agree this is not a good idea. |
4 participants
Test cases: openresty/resty-cli#38