Fix: Proper error handling for file:open() call in couch_file.erl#1164
Conversation
|
-1. We assert that we get the Adding error handling code for errors we don't intend to handle (in the sync case, you immediately throw it anyway) is an anti-pattern in erlang, though common (or mandatory) in other languages. |
|
@davisp if you would like to comment on the init portions. |
davisp
left a comment
davisp
left a comment
There was a problem hiding this comment.
Looks good for the most part. Just a few style nits that need straightening out.
| {ok, Fd} = file:open(Filepath, [append, raw]), | ||
| try ok = file:sync(Fd) after ok = file:close(Fd) end; | ||
| case file:open(Filepath, [append, raw]) of | ||
| {ok, Fd} -> |
There was a problem hiding this comment.
Use four space indents everywhere.
| {ok, Fd} -> | ||
| try ok = file:sync(Fd) after ok = file:close(Fd) end; | ||
| Error -> | ||
| throw(Error) |
There was a problem hiding this comment.
Match {error, Error} and then erlang:error(Error) here.
|
@rnewson yes and no. The issue is where we want that badmatch to happen and with what as the badmatch. For the init section this is a good change as it matches the initial open. What's being added is the handling when the second of two fails to behave the same as the first. This is fine as we already handle the error. And it cleans up logs a bit in that the emfile error happens in the process doing the open rather than a EXIT/DOWN message when the couch_file pid dies on a badmatch. As for the init process this just reuses the existing logic which seems cleaner than going back and rewriting it to use proc_lib properly. +1 after style issues are addressed |
|
if @davisp is happy, I'm happy. |
| try ok = file:sync(Fd) after ok = file:close(Fd) end; | ||
| {error, Error} -> | ||
| erlang:error(Error) | ||
| end; |
There was a problem hiding this comment.
This end should be dedented one level.
| {ok, Eof} = file:position(Fd, eof), | ||
| erlang:send_after(?INITIAL_WAIT, self(), maybe_close), | ||
| {ok, #file{fd=Fd, eof=Eof, is_sys=IsSys, pread_limit=Limit}}; | ||
| case file:open(Filepath, OpenOptions) of |
| {ok, #file{fd=Fd, eof=Eof, is_sys=IsSys, pread_limit=Limit}}; | ||
| Error -> | ||
| init_status_error(ReturnPid, Ref, Error) | ||
| end; |
4 participants
Overview
According to Erlang's file:open docs this function returns
{error, Reason}in case of failure but insidecouch_file:syncandinitfunctions there are no checks for this in two places. Consequently when I received{error, emfile}kind of error, process crashed withbadmatchinstead of a proper error.Testing recommendations
Simulate
{error, emfile}conditions (e.g. decreaseulimiton your OS) and observe an error.Checklist