Skip to content
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

server: unify accept error handling #1882

Merged
merged 1 commit into from
Aug 23, 2024
Merged

server: unify accept error handling #1882

merged 1 commit into from
Aug 23, 2024

Conversation

djc
Copy link
Collaborator

@djc djc commented Aug 21, 2024

Motivation

Share error handling logic from #1874 between TCP and TLS accept loops.

cc @grembo

@grembo
Copy link
Contributor

grembo commented Aug 21, 2024

@djc Simple and elegant. Not sure if "accept_error" is the best name for the function (given that could be read as accepting an error or converting an error in accept). Maybe something like "assess_accept_error" would be better.

The one thing I'm wondering about is if the "never fail" approach of the tls enabled loop might have masked other errors that appear in practice (e.g., we don't run these services with tracing disabled, so we won't know).

Potential candidates:

     [EINTR]            The accept() operation was interrupted.

     [EMFILE]           The per-process descriptor table is full.

     [ENFILE]           The system file table is full.
     
     [EWOULDBLOCK] or [EAGAIN]
                              The socket is marked non-blocking and no connections
                              are present to be accepted.

I didn't find user code that handles EINTR in accept, so I guess that's not a thing in practice (edit: still, ignoring it won't hurt).

Most programs will capsize in case of EMFILE and ENFILE anyway (some resistent daemons handle it though by delaying the next loop iteration), so some services would benefit from it.

EWOULDBLOCK is handled inside tokio/net, but it seems like EAGAIN isn't.

So based on this quick "analysis", I would suggest to handle at least EAGAIN and EINTR (edited) in here as well (unless I missed something, which is quite possible).

To make sure, I checked the source of squid proxy (which is around long enough to have these things covered), which does this:

    const auto rawSock = accept(conn->fd, gai->ai_addr, &gai->ai_addrlen);
    if (rawSock < 0) {
        errcode = errno; // store last accept errno locally.

        Ip::Address::FreeAddr(gai);

        if (ignoreErrno(errcode) || errcode == ECONNABORTED) {
            debugs(50, 5, status() << ": " << xstrerr(errcode));
            return false;
        } else {
            throw TextException(ToSBuf("Failed to accept an incoming connection: ", xstrerr(errcode)), Here());
        }
    }

and

/*
 * hm, this might be too general-purpose for all the places we'd
 * like to use it.
 */
int
ignoreErrno(int ierrno)
{
    switch (ierrno) {

    case EINPROGRESS:

    case EWOULDBLOCK:
#if EAGAIN != EWOULDBLOCK

    case EAGAIN:
#endif

    case EALREADY:

    case EINTR:
#ifdef ERESTART

    case ERESTART:
#endif

        return 1;

    default:
        return 0;
    }

    /* NOTREACHED */
}

@djc
Copy link
Collaborator Author

djc commented Aug 23, 2024

Changed the name to handle_accept_error(). Will defer adding more error kinds to another PR.

@djc djc added this pull request to the merge queue Aug 23, 2024
Merged via the queue into master with commit c3be20c Aug 23, 2024
16 checks passed
@djc djc deleted the accept-errors branch August 23, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants