slightly simplify accept error-handling logic (twitter#210)

* slightly simplify accept error-handling logic

* move asserts to public functions

* add another assert

* undo previous change and add comment on what happens on various errors

src/channel/cc_tcp.c

@@ -319,33 +319,63 @@ _tcp_accept(struct tcp_conn *sc)
 {
     int sd;
 
-    ASSERT(sc->sd > 0);
-
-    for (;;) { /* we accept at most one tcp_conn with the 'break' at the end */
+    ASSERT(sc->sd >= 0);
+
+    /* How does tcp accept work when a separate thread is used to accept new
+     * connections?
+     *
+     * In general, we want to accept a new connection at a time (on the server
+     * thread), then hand this connection over to be put on some other event
+     * loop (e.g. on a worker thread's), and some additional preparation may
+     * be necessary (e.g. allocating R/W buffers). This is why we break after
+     * completing a single `accept' successfully.
+     *
+     * There are several ways `accept' could "fail", and they need to be
+     * treated differently. The most common case, which isn't really a failure
+     * on a non-blocking socket, is receiving EAGAIN/EWOULDBLOCK. This simply
+     * means there is no new connection to accept and the function should
+     * return.
+     * EINTR is another common error which means the call was terminated by
+     * a signal. This type of interruption is almost always transient, so
+     * an immediate retry is likely to succeed.
+     * The rest of the exceptions likely to occur on a SOCK_STREAM socket are
+     * often due to exhaustion of some resources (e.g. fd, memory), and there
+     * is no guarantee they will recover immediately. For example, to free
+     * up another fd requires an existing connection to be closed. In such
+     * cases, the connection in the backlog will sit there (as fully
+     * established as far as TCP stack is concerned) until accept in application
+     * becomes possible again, and any new connections arriving will be added
+     * to the back of the queue until it's full, at which point the client
+     * will receive an exception and the connect attempt will fail.
+     */
+    for (;;) {
 #ifdef CC_ACCEPT4
         sd = accept4(sc->sd, NULL, NULL, SOCK_NONBLOCK);
 #else
         sd = accept(sc->sd, NULL, NULL);
 #endif /* CC_ACCEPT4 */
         if (sd < 0) {
-            if (errno == EINTR) {
-                log_debug("accept on sd %d not ready: eintr", sc->sd);
-                continue;
-            }
-
             if (errno == EAGAIN || errno == EWOULDBLOCK) {
                 log_debug("accept on sd %d not ready: eagain", sc->sd);
                 return -1;
             }
 
+            if (errno == EINTR) {
+                log_debug("accept on sd %d not ready: eintr", sc->sd);
+
+                continue;
+            }
+
             log_error("accept on sd %d failed: %s", sc->sd, strerror(errno));
             INCR(tcp_metrics, tcp_accept_ex);
-            continue;
+
+            return -1;
         }
 
         break;
     }
 
+    ASSERT(sd >= 0);
     return sd;
 }
 
@@ -423,19 +453,21 @@ tcp_reject_all(struct tcp_conn *sc)
     for (;;) {
         sd = accept(sc->sd, NULL, NULL);
         if (sd < 0) {
-            if (errno == EINTR) {
-                log_debug("sd %d not ready: eintr", sc->sd);
-                continue;
-            }
-
             if (errno == EAGAIN || errno == EWOULDBLOCK) {
                 log_debug("sd %d has no more outstanding connections", sc->sd);
                 return;
             }
 
+            if (errno == EINTR) {
+                log_debug("accept on sd %d not ready: eintr", sc->sd);
+
+                continue;
+            }
+
             log_error("accept on sd %d failed: %s", sc->sd, strerror(errno));
             INCR(tcp_metrics, tcp_reject_ex);
-            return;
+
+            return -1;
         }
 
         ret = close(sd);

src/event/cc_epoll.c

@@ -122,9 +122,6 @@ _event_update(struct event_base *evb, int fd, int op, uint32_t events, void *ptr
 {
     struct epoll_event event;
 
-    ASSERT(evb != NULL && evb->ep > 0);
-    ASSERT(fd > 0);
-
     event.events = events;
     event.data.ptr = ptr;
 
@@ -135,6 +132,9 @@ int event_add_read(struct event_base *evb, int fd, void *data)
 {
     int status;
 
+    ASSERT(evb != NULL && evb->ep > 0);
+    ASSERT(fd >= 0);
+
     /*
      * Note(yao): there have been tests showing EPOLL_CTL_ADD is cheaper than
      * EPOLL_CTL_MOD, and the only difference is we need to ignore EEXIST
@@ -156,6 +156,9 @@ event_add_write(struct event_base *evb, int fd, void *data)
 {
     int status;
 
+    ASSERT(evb != NULL && evb->ep > 0);
+    ASSERT(fd >= 0);
+
     status = _event_update(evb, fd, EPOLL_CTL_ADD, EPOLLOUT, data);
     if (status < 0 && errno != EEXIST) {
         log_error("ctl (add write) w/ epoll fd %d on fd %d failed: %s", evb->ep,
@@ -173,6 +176,9 @@ event_del(struct event_base *evb, int fd)
 {
     int status;
 
+    ASSERT(evb != NULL && evb->ep > 0);
+    ASSERT(fd >= 0);
+
     /* event can be NULL in kernel >=2.6.9, here we keep it for compatibility */
     status = _event_update(evb, fd, EPOLL_CTL_DEL, 0, NULL);
     if (status < 0) {