-
Notifications
You must be signed in to change notification settings - Fork 197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bugfix: resume read system call upon EINTR during file readall. #16
Conversation
1fcaa65
to
d0f67c2
Compare
@thibaultcha Awesome. Thanks! Will you also have a shot on proposing the same PR to the mainstream LuaJIT repo below at the same time? https://github.com/LuaJIT/LuaJIT/pulls It would be great if this can be accepted upstream :) |
src/lib_io.c
Outdated
n += (MSize)fread(buf+n, 1, m-n, fp); | ||
if (n == m) break; | ||
else if (ferror(fp) != 0) { | ||
if (errno != EINTR) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should use break
instead of return
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, break
is not right either. Better return the partially read data here before returning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to? It seems to me like the only code path we are following after this is luaL_fileresult which returns nil, errno, string
to the Lua-land? We also avoid repeating the GC step).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, maybe we do need the GC step anyway for the buffer allocated above. I originally wasn't sure it was needed, but I do think so now.
I agree, yes I will! Making sure it is square first :) |
@thibaultcha I should be a bugfix, I believe. |
Ha, I’ll re-rename it back then. |
d0f67c2
to
1837686
Compare
Renamed the commit and updated the |
1837686
to
11c4ae0
Compare
I just updated the patch to run a GC step upon error. I believe it is close to completion now (and the test case has been improved as per your request a few days ago). Mind taking a look? If it looks ok, I will submit it against the upstream repository as discussed. |
src/lib_io.c
Outdated
for (;;) { | ||
n += (MSize)fread(buf+n, 1, m-n, fp); | ||
if (n == m) break; | ||
else if (ferror(fp)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can save the else
word here since the previous branch never falls through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch is to prevent adding the read data to the stack upon an error, because I believe the previous behavior was an oversight due to the previous design of this function, which had no error handling at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thibaultcha OK, I see we now have proper error handling for general file I/O errors. Huh, more than I expected :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thibaultcha The else
here does not make any difference, no? the previous branch never falls through (it always breaks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, on mobile (still) I just realized this is the above branch you are talking about. I’ll gladly remove the else
here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
src/lib_io.c
Outdated
n += (MSize)fread(buf+n, 1, m-n, fp); | ||
if (n == m) break; | ||
else if (ferror(fp)) { | ||
if (errno == EINTR) { clearerr(fp); continue; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous behavior is to read until the end of the stream or until an error occurred. here we throw away all the already read data when an error happens. This looks wrong to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t see any path when an error occurs where this value would be used. From my understanding, the previous behavior simply did not consider errors, and thus did not need to deal with such cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
io_file_readall() is a helper function of io_file_read() which check if error take place.
Interruption should not be considered as a real error, also it's difficult to resume read/write in io_file_read(), so I guess this piece of code needs some change.
That being said, I don't think this fix is enough. For one thing, other helper function (say io_file_readnum()) have similar problem, and this patch does not cover them. On the other hand, this is the problem specific the OR environment. Maybe you can explain the problem to Luajit mailing list and solicit comment from broader audiences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this fix is only contained to readall. I know there are other areas that require similar attention.
11c4ae0
to
970edb8
Compare
@thibaultcha Merged. Thanks! |
Awesome! Back tomorrow, I will try to push this into upstream as well as maybe fixing a few other places. Thanks! |
Patch proposed in #15, ported to its own PR. Test case located at openresty/resty-cli#37.