Discard out_channel buffered data on permanent I/O error#12314
Discard out_channel buffered data on permanent I/O error#12314xavierleroy merged 4 commits intoocaml:trunkfrom
Conversation
| case ERROR_CURRENT_DIRECTORY: case ERROR_BUSY: | ||
| errno = EBUSY; break; |
There was a problem hiding this comment.
The new code maps ERROR_CURRENT_DIRECTORY to EACCES, not EBUSY, for consistency with _dosmaperr and the Win32 Unix library. It's a small change to map it back to EBUSY, but I'm not sure it's worth departing from _dosmaperr here.
nojb
left a comment
There was a problem hiding this comment.
Did a first pass over the code, could only spot one potential issue; question below.
nojb
left a comment
There was a problem hiding this comment.
LGTM, could not spot any other issues.
|
One question is whether there are other error codes that should be handled in the same way as |
|
I went through
|
| { ERROR_LOCK_FAILED, 0, EACCES}, | ||
| { ERROR_ALREADY_EXISTS, 0, EEXIST}, | ||
| { ERROR_FILENAME_EXCED_RANGE, 0, ENOENT}, | ||
| { ERROR_NESTING_NOT_ALLOWED, 0, EAGAIN}, |
There was a problem hiding this comment.
Returning EAGAIN on ERROR_NESTING_NOT_ALLOWED doesn't seem to fit the "resource temporarily unavailable" meaning. Wouldn't this potentially cause an infinite loop, since retrying won't fix the fact that there are nested LoadModule invocations earlier in the callchain. But Python's win32_to_errno function also does the same mapping to EAGAIN, so I'm probably wrong (but not sure why).
(I read through these mappings while reading the rest of the diff; possibly better as a followup PR if a change is needed in these mappings)
There was a problem hiding this comment.
The _dosmaperr function from the CRT maps ERROR_NESTING_NOT_ALLOWED to EAGAIN. As with ERROR_CURRENT_DIRECTORY, we could choose a different mapping, but do we really want to diverge from the CRT?
|
@dra27 do you have an opinion on the Windows part of this PR, esp. the mapping from Win32 error codes to POSIX error codes? |
79cb8b4 to
f2f8a2c
Compare
Use it in `caml_win32_rename`.
In case of error, they just return -1 after setting errno to an appropriate POSIX error code. The caller is responsible for processing the error. Typically, EINTR handles the signal and retries, other errors raise Sys_error.
… I/O error So that the out_channel can be reclaimed by finalization.
9bfa1b5 to
ed4d191
Compare
|
Re-based, re-tested, and merged. The Win32 part, and especially the mapping of Win32 error codes, can still be discussed, I'm all ears. |
|
I believe this PR introduced a regression, see #12898 for details. |
This PR explores the third approach mentioned here: #12300 (comment) . Implementing it led me to refactor some I/O code in the runtime system, but the new code is an improvement, if I may say so myself.
This PR is best reviewed commit-by-commit.
The third commit changes the
flushoperation overout_channelto discard the buffered data if the actual write fails with an EBADF ("bad file descriptor", including "the file descriptor was closed") or EPIPE ("pipe was closed at the other end") error. In both cases, retrying the write will fail, it's not a transient error condition like ENOSPC ("no space left on device"). So, before raising Sys_error, we just remove the buffered data.This way, if the channel is finalized later, it will appear as empty (not containing unflushed data) and it will be freed immediately. This avoids the memory leak reported in #12300. It also avoids keeping the channel around and trying to flush it again when the program exits, which can lead to writing to the wrong file, as shown in #12300.
To implement this behavior, I had to change the interface of
caml_read_fdandcaml_write_fd. Currently, they return an error code for EINTR and directly raise a Sys_error exception for any other error. The second commit changes them to return -1 on any error and set errno to the error code, like a POSIX system call. It's now the caller's responsibility to act on the error, either by retrying, or by raising Sys_error, or by emptying an out_channel then raising Sys_error.But there's a snag in the Windows implementation of
caml_{read,write}_fd. They either call CRT functions, which leave POSIX-style error codes in errno, or Win32 socket functions, which produce Win32 error codes viaWSAGetLastError(). So we need to convert from Win32 error codes to POSIX error codes. There's a_dosmaperrfunction in the CRT that does just this but is not exported. There's acaml_win32_maperrfunction in otherlibs/unix that does just this but we need it in the runtime. So, the first commit bites the bullet and moves the core of thecaml_win32_maperrfunction from otherlibs/unix/unixsupport_win32.c to runtime/win32.c, where it can be used bycaml_{read,write}_fdand also bycaml_win32_rename. Phew!