Bug 117266 – [WK2] Looping for EINTR on close() is incorrect for Linux, at least

• Component: WebKit2
• Status: RESOLVED FIXED

[Sergio Correia (qrwteyrutiyoup)]

[WK2]: Do not loop for EINTR on close()

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 203869 [details]
Patch

RFC patch, not yet for review.

[Darin Adler]

Comment on attachment 203869 [details]
Patch

Given the discussion in the change log it sounds like this change is wrong. On platforms that have “the HPUX behavior” these file descriptors will be left open. And once the recommendation of the Austin group is implemented, the code will be correct on the other platforms too.

If we need to work around the current Linux behavior, it seems to me we should call through a helper function that does the right thing for Linux. Changing all the call sites to ignore the issue and leave the file descriptors open if interrupted seems clearly the wrong thing to do.

Am I missing something? There’s all that data about what the situation is, and then a change that seems to ignore the data.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #2)
> (From update of attachment 203869 [details])
> Given the discussion in the change log it sounds like this change is wrong. On platforms that have “the HPUX behavior” these file descriptors will be left open. And once the recommendation of the Austin group is implemented, the code will be correct on the other platforms too.
> 
> If we need to work around the current Linux behavior, it seems to me we should call through a helper function that does the right thing for Linux. Changing all the call sites to ignore the issue and leave the file descriptors open if interrupted seems clearly the wrong thing to do.
> 
> Am I missing something? There’s all that data about what the situation is, and then a change that seems to ignore the data.

Nope, you are not missing anything. This first patch was mostly to bring attention to the issue. 

I also agree that having a helper take care of it seems better, as we can centralize any quirks some platforms might need, and we could provide also some background on the situation. Any ideas on where we could define such helper?

Thanks for the feedback.

[Darin Adler]

(In reply to comment #3)
> I also agree that having a helper take care of it seems better, as we can centralize any quirks some platforms might need, and we could provide also some background on the situation. Any ideas on where we could define such helper?

Somewhere in WTF might be OK. Maybe a file named <wtf/UniStdExtras.h>? Or in WebKit2/Platform. Not sure what file name I’d use there.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 204442 [details]
Patch

Added a UniStdExtras class with a wrapper around close(), to handle EINTR correctly for Linux and use this new function in several call sites across WK2 code where we were looping for EINTR on close().)

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 204849 [details]
Patch

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #4)
> (In reply to comment #3)
> > I also agree that having a helper take care of it seems better, as we can centralize any quirks some platforms might need, and we could provide also some background on the situation. Any ideas on where we could define such helper?
> 
> Somewhere in WTF might be OK. Maybe a file named <wtf/UniStdExtras.h>? Or in WebKit2/Platform. Not sure what file name I’d use there.

I have created a UniStdExtras.h in WTF, as per your suggestion, and added a wrapper around the close() call. Could you take a look at the current patch?

[Darin Adler]

Comment on attachment 204849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204849&action=review

Looks OK, needs to not be a class, though.

> Source/WTF/wtf/UniStdExtras.h:34
> +class UniStdExtras {

This should not be a class.

> Source/WTF/wtf/UniStdExtras.h:36
> +    static int close(int fileDescriptor)

Should just have a global function with a descriptive name. Something like closeWithRetry maybe.

> Source/WTF/wtf/UniStdExtras.h:53
> +} // namespace WTF

Need the using that we do in WTF. Like this:

    using WTF::closeWithRetry;

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #8)
> (From update of attachment 204849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204849&action=review
> 
> Looks OK, needs to not be a class, though.
> 
> > Source/WTF/wtf/UniStdExtras.h:34
> > +class UniStdExtras {
> 
> This should not be a class.
> 
> > Source/WTF/wtf/UniStdExtras.h:36
> > +    static int close(int fileDescriptor)
> 
> Should just have a global function with a descriptive name. 

OK, I will update it.

> Something like closeWithRetry maybe.

How about something even more descriptive like closeHandlingEINTR? It might be more obvious to people reading the code. closeWithRetry seems much better than just close, anyway, so if you feel strongly about it, we can use closeWithRetry.

> 
> > Source/WTF/wtf/UniStdExtras.h:53
> > +} // namespace WTF
> 
> Need the using that we do in WTF. Like this:
> 
>     using WTF::closeWithRetry;

Noted.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #8)
> (From update of attachment 204849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204849&action=review
> 
> Looks OK, needs to not be a class, though.
> 
> > Source/WTF/wtf/UniStdExtras.h:34
> > +class UniStdExtras {
> 
> This should not be a class.
> 
> > Source/WTF/wtf/UniStdExtras.h:36
> > +    static int close(int fileDescriptor)
> 
> Should just have a global function with a descriptive name. Something like closeWithRetry maybe.
> 

Oh, and a last question: to avoid having the function being redefined, would you prefer this global function to keep being static, to be marked inline or to be defined in a source cpp file?

Thanks!

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 205002 [details]
Patch

Using closeWithRetry() as a more descriptive name and not creating a class anymore, as per Darin's suggestions.

[Darin Adler]

Comment on attachment 205002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205002&action=review

> Source/WebKit2/ChangeLog:44
> +        POSIX says the following for close():
> +
> +        If close() is interrupted by a signal that is to be caught, it shall
> +        return -1 with errno set to [EINTR] and the state of fildes is
> +        unspecified.
> +
> +        Different implementations do different things when close() is interrupted
> +        by a signal: HP-UX leaves the file descriptor open while returning EINTR,
> +        while Linux and AIX, for instance, close the descriptor unconditionally.
> +
> +        As someone pointed out in the discussion in [1], the actual problem with
> +        this behavior is not knowing how to safely proceed when getting EINTR
> +        during a close() call. In single-threaded applications, there's not much of
> +        a problem, since one could just call close() again and get EBADF if the
> +        descriptor had been closed the first time; however, in multithreaded
> +        applications it becomes dangerous to call close() again, since it's
> +        possible s subsequent call could close a descriptor just obtained by
> +        another thread.
> +
> +        As noted in [5], the Austin Group resolved the issue [6] by requiring
> +        the HPUX behavior if close returns with EINTR, but allowing implementations
> +        which want to deallocate the file descriptor even if interrupted by a
> +        signal to return with the EINPROGRESS error instead of EINTR.
> +
> +        The ``while (close(fd) == -1 && errno == EINTR) { }'' idiom seems to be
> +        very popular, especially in WK2 code, and since it's the wrong thing to do
> +        at least on Linux, this patch is changing these sites to use a new function
> +        closeWithRetry() added in this commit, which currently works around the
> +        described Linux behavior.
> +
> +        Some links for reference:
> +        [1] https://news.ycombinator.com/item?id=3363819
> +        [2] http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
> +        [3] https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain
> +        [4] http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR
> +        [5] http://ewontfix.com/4/
> +        [6] http://austingroupbugs.net/view.php?id=529

Comment is too big. The comment in WTF/ChangeLog is better. Just use something like that. “Call closeWithRetry to work around a difference in how the retry needs to be done on Linux.”

> Source/WTF/wtf/UniStdExtras.h:2
> +#ifndef UniStdExtras_h
> +#define UniStdExtras_h

These header guards go after the comment in the WebKit coding style.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 205142 [details]
Patch

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #12)
> (From update of attachment 205002 [details])
> 
> Comment is too big. The comment in WTF/ChangeLog is better. Just use something like that. “Call closeWithRetry to work around a difference in how the retry needs to be done on Linux.”
> 
> > Source/WTF/wtf/UniStdExtras.h:2
> > +#ifndef UniStdExtras_h
> > +#define UniStdExtras_h
> 
> These header guards go after the comment in the WebKit coding style.

Thanks for the review. I have updated the comment in the WebKit2/ChangeLog and the header guards to conform with the WebKit coding style.

[WebKit Commit Bot]

Comment on attachment 205142 [details]
Patch

Clearing flags on attachment: 205142

Committed r151825: <http://trac.webkit.org/changeset/151825>