httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Roy T. Fielding" <field...@kiwi.ICS.UCI.EDU>
Subject [PATCH] Big fixes to main loop, lingering_close
Date Sat, 15 Feb 1997 18:42:51 GMT
I spent the night carefully going through the the main child loop
and lingering_close.  Aside from lack of comments and whitespace hell,
the problems include:

   1) Local overrides of global variable names (sd) are a bad idea,
      particularly when they are used with signal handlers.

   2) Reusing the same variable (csd) for multiple purposes is a
      very bad idea, particularly when those purposes are as the return
      values from two different system functions (select and accept),
      since that naturally leads to ...

   3) Forgetting that 0 is a valid return value for accept().

   4) Leaving huge debugging fprintfs around are a bad idea, even when
      commented out, because they obscure bugs like ...

   5) Failing to call sync_scoreboard_image() before checking
      if (scoreboard_image->global.exit_generation >= generation).

   6) Duplicating 100% of the select and accept process when only
      10% is unique is a bad idea, particularly when the two methods
      (listeners vs single socket) are implemented using arbitrarily
      different loop mechanisms, since it leads to really stupid
      conditions like ...

   7) Failing to check the same conditions and exit_generation in
      both loops, and allowing an error condition to fall-through
      or the child to ignore graceful restarts, and making it difficult
      to add things like the check of EPROTO and ECONNABORTED.

   8) Duplicating 50% of the reading requests process is a bad idea,
      because it makes it much harder to understand the code, is
      equivalent to a different form of loop with no duplication,
      and hides bugs like...

   9) Doing sync_scoreboard_image after the second request instead
      of between every request.

  10) Flushing the buffers twice on a bclose is just a waste of time.

  11) Closing a socket with close(), and thus without reclaiming the
      dupped descriptor that would have been closed by bclose(), is
      just a waste of resources.

  12) Failing to check all of the possible aborted connection
      side-effects before calling lingering_close could lead to
      null pointer references and failure to close at all.

  13) Logging an error message upon the final close, when we are forcing
      a close on purpose and don't care about any errors, will just
      annoy our users.

Needless to say, this section of code is critical, and this patch needs
serious review ASAP.  It may fix several reported problems, including
missing status updates, graceful restarts, leaking file descriptors,
and lingering_close.  OTOH, it may fix none of them, but it will be
worth it just to improve readability.

Regarding Marc's earlier question, I think FNDELAY is portable to any
system that implements sockets.  If it isn't, then we can add ifdefs,
but we can't just remove the non-blocking (we must replace it with the
local equivalent, or set NO_LINGCLOSE).

.....Roy

Index: http_main.c
===================================================================
RCS file: /export/home/cvs/apache/src/http_main.c,v
retrieving revision 1.120
diff -c -r1.120 http_main.c
*** http_main.c	1997/02/15 12:26:20	1.120
--- http_main.c	1997/02/15 17:07:20
***************
*** 313,355 ****
  {
      int dummybuf[512];
      struct timeval tv;
!     fd_set fds, fds_read, fds_err;
      int select_rv = 0, read_rv = 0;
!     int sd = r->connection->client->fd;
  
!     if (sd < 0)          /* Don't do anything if the socket is invalid */
!         return;
  
!     kill_timeout(r);     /* Remove any leftover timeouts */
  
!     /* Close our half of the connection --- send client a FIN and
       * set the socket to non-blocking for later reads.
       */
  
!     if (((shutdown(sd, 1)) != 0) || (fcntl(sd, F_SETFL, FNDELAY) == -1)) {
  	/* if it fails, no need to go through the rest of the routine */
  	if (errno != ENOTCONN)
  	    log_unixerr("shutdown", NULL, "lingering_close", r->server);
! 	close(sd);
  	return;
      }
  
      /* Set up to wait for readable data on socket... */
  
!     FD_ZERO(&fds);
!     FD_SET(sd, &fds);
  
      /* Wait for readable data or error condition on socket;
       * slurp up any data that arrives...  We exit when we go for 
       * an interval of tv length without getting any more data, get an
!      * error from select(), get an exception on sd, get an error or EOF
       * on a read, or the timer expires.
       */
  
-     /* Prevent a slow-drip client from holding us here indefinitely */
- 
-     hard_timeout("lingering_close", r);
- 
      do {
          /* We use a 1 second timeout because current (Feb 97) browsers
           * fail to close a connection after the server closes it.  Thus,
--- 313,357 ----
  {
      int dummybuf[512];
      struct timeval tv;
!     fd_set lfds, fds_read, fds_err;
      int select_rv = 0, read_rv = 0;
!     int lsd;
  
!     /* Prevent a slow-drip client from holding us here indefinitely */
! 
!     kill_timeout(r);                 /* Remove any leftover timeouts */
!     hard_timeout("lingering_close", r);
! 
!     /* Send any leftover data to the client, but never try to again */
  
!     bflush(r->connection->client);
!     bsetflag(r->connection->client, B_EOUT, 1);
  
!     /* Close our half of the connection --- send the client a FIN and
       * set the socket to non-blocking for later reads.
       */
+     lsd = r->connection->client->fd;
  
!     if (((shutdown(lsd, 1)) != 0) || (fcntl(lsd, F_SETFL, FNDELAY) == -1)) {
  	/* if it fails, no need to go through the rest of the routine */
  	if (errno != ENOTCONN)
  	    log_unixerr("shutdown", NULL, "lingering_close", r->server);
! 	bclose(r->connection->client);
  	return;
      }
  
      /* Set up to wait for readable data on socket... */
  
!     FD_ZERO(&lfds);
!     FD_SET(lsd, &lfds);
  
      /* Wait for readable data or error condition on socket;
       * slurp up any data that arrives...  We exit when we go for 
       * an interval of tv length without getting any more data, get an
!      * error from select(), get an exception on lsd, get an error or EOF
       * on a read, or the timer expires.
       */
  
      do {
          /* We use a 1 second timeout because current (Feb 97) browsers
           * fail to close a connection after the server closes it.  Thus,
***************
*** 363,380 ****
          tv.tv_sec  = 1;
          tv.tv_usec = 0;
          read_rv    = 0;
!         fds_read   = fds;
!         fds_err    = fds;
      
  #ifdef SELECT_NEEDS_CAST
!         select_rv = select(sd + 1, (int*)&fds_read, NULL, (int*)&fds_err, &tv);
  #else
!         select_rv = select(sd + 1, &fds_read, NULL, &fds_err, &tv);
  #endif
      } while ((select_rv > 0) &&           /* Something to see on socket    */
!              !FD_ISSET(sd, &fds_err) &&   /* that isn't an error condition */
!              FD_ISSET(sd, &fds_read) &&   /* and is worth trying to read   */
!              ((read_rv = read(sd, dummybuf, sizeof dummybuf)) > 0));
  
      /* Log any errors that occurred (client close or reset is not an error) */
      
--- 365,382 ----
          tv.tv_sec  = 1;
          tv.tv_usec = 0;
          read_rv    = 0;
!         fds_read   = lfds;
!         fds_err    = lfds;
      
  #ifdef SELECT_NEEDS_CAST
!         select_rv = select(lsd+1, (int*)&fds_read, NULL, (int*)&fds_err, &tv);
  #else
!         select_rv = select(lsd+1, &fds_read, NULL, &fds_err, &tv);
  #endif
      } while ((select_rv > 0) &&           /* Something to see on socket    */
!              !FD_ISSET(lsd, &fds_err) &&   /* that isn't an error condition
*/
!              FD_ISSET(lsd, &fds_read) &&   /* and is worth trying to read  
*/
!              ((read_rv = read(lsd, dummybuf, sizeof dummybuf)) > 0));
  
      /* Log any errors that occurred (client close or reset is not an error) */
      
***************
*** 385,392 ****
  
      /* Should now have seen final ack.  Safe to finally kill socket */
  
!     if (close(sd) < 0)
!         log_unixerr("close", NULL, "lingering_close", r->server);
  
      kill_timeout(r);
  }
--- 387,393 ----
  
      /* Should now have seen final ack.  Safe to finally kill socket */
  
!     bclose(r->connection->client);
  
      kill_timeout(r);
  }
***************
*** 1521,1530 ****
--- 1522,1533 ----
   * they are really private to child_main.
   */
  
+ static int srv;
  static int csd;
  static int dupped_csd;
  static int requests_this_child;
  static int child_num;
+ static fd_set main_fds;
  
  void child_main(int child_num_arg)
  {
***************
*** 1540,1547 ****
      dupped_csd = -1;
      child_num = child_num_arg;
      requests_this_child = 0;
!     reopen_scoreboard (pconf);
!     (void)update_child_status (child_num, SERVER_READY, (request_rec*)NULL);
  
  #ifdef MPE
      /* Only try to switch if we're running as MANAGER.SYS */
--- 1543,1551 ----
      dupped_csd = -1;
      child_num = child_num_arg;
      requests_this_child = 0;
! 
!     reopen_scoreboard(pconf);
!     (void)update_child_status(child_num, SERVER_READY, (request_rec*)NULL);
  
  #ifdef MPE
      /* Only try to switch if we're running as MANAGER.SYS */
***************
*** 1551,1557 ****
              GETUSERMODE();
  #else
      /* Only try to switch if we're running as root */
!     if(!geteuid() && setuid(user_id) == -1) {
  #endif
          log_unixerr("setuid", NULL, "unable to change uid", server_conf);
  	exit (1);
--- 1555,1561 ----
              GETUSERMODE();
  #else
      /* Only try to switch if we're running as root */
!     if (!geteuid() && setuid(user_id) == -1) {
  #endif
          log_unixerr("setuid", NULL, "unable to change uid", server_conf);
  	exit (1);
***************
*** 1561,1566 ****
--- 1565,1574 ----
      }
  #endif
  
+     /*
+      * Setup the jump buffers so that we can return here after
+      * a signal or a timeout (yeah, I know, same thing).
+      */
  #ifdef NEXT
      setjmp(jmpbuffer);
  #else
***************
*** 1574,1579 ****
--- 1582,1591 ----
  	BUFF *conn_io;
  	request_rec *r;
        
+         /*
+          * (Re)initialize this child to a pre-connection state.
+          */
+ 
          alarm(0);		/* Cancel any outstanding alarms. */
          timeout_req = NULL;	/* No request in progress */
  	current_conn = NULL;
***************
*** 1582,1590 ****
  	clear_pool (ptrans);
  	
  	sync_scoreboard_image();
! 
! 	/*fprintf(stderr,"%d check %d %d\n",getpid(),scoreboard_image->global.exit_generation,generation);*/
! 	if(scoreboard_image->global.exit_generation >= generation)
  	    exit(0);
  	
  	if ((count_idle_servers() >= daemons_max_free)
--- 1594,1600 ----
  	clear_pool (ptrans);
  	
  	sync_scoreboard_image();
! 	if (scoreboard_image->global.exit_generation >= generation)
  	    exit(0);
  	
  	if ((count_idle_servers() >= daemons_max_free)
***************
*** 1594,1676 ****
  	    exit(0);
  	}
  
! 	(void)update_child_status (child_num, SERVER_READY, (request_rec*)NULL);
! 	
! 	accept_mutex_on();  /* Lock around "accept", if necessary */
! 
! 	if (listeners != NULL)
! 	{
! 	    fd_set fds;
  
! 	    for (;;) {
! 		memcpy(&fds, &listenfds, sizeof(fd_set));
! #ifdef SELECT_NEEDS_CAST
! 		csd = select(listenmaxfd+1, (int*)&fds, NULL, NULL, NULL);
! #else
!                 csd = select(listenmaxfd+1, &fds, NULL, NULL, NULL);
! #endif
! 		if (csd < 0 && errno != EINTR)
! 		    log_unixerr("select",NULL,"select error", server_conf);
  
! 		/*fprintf(stderr,"%d check(2a) %d %d\n",getpid(),scoreboard_image->global.exit_generation,generation);*/
! 		if(scoreboard_image->global.exit_generation >= generation)
! 		    exit(0);
  
! 		if (csd <= 0) continue;
! 		for (sd=listenmaxfd; sd >= 0; sd--)
! 		    if (FD_ISSET(sd, &fds)) break;
! 		if (sd < 0) continue;
! 
!                 do {
!                     clen = sizeof(sa_client);
!                     csd  = accept(sd, &sa_client, &clen);
!                 } while (csd < 0 && errno == EINTR);
!                 if (csd > 0) break;
! 		log_unixerr("accept", "(client socket)", NULL, server_conf);
! 	    }
! 	}
! 	else {
! 	    fd_set fds;
  
! 	    do {
!                 FD_ZERO(&fds);
!                 FD_SET(sd,&fds);
  #ifdef SELECT_NEEDS_CAST
! 		csd = select(sd+1, (int*)&fds, NULL, NULL, NULL);
  #else
! 		csd = select(sd+1, &fds, NULL, NULL, NULL);
  #endif
!             } while (csd < 0 && errno == EINTR);
  
!             if (csd < 0) {
! 		log_unixerr("select","(listen)",NULL,server_conf);
! 		exit(0);
              }
- 	    /*fprintf(stderr,"%d check(2a) %d %d\n",getpid(),scoreboard_image->global.exit_generation,generation);*/
- 	    sync_scoreboard_image();
- 	    if(scoreboard_image->global.exit_generation >= generation)
- 		exit(0);
  
              do {
                  clen = sizeof(sa_client);
                  csd  = accept(sd, &sa_client, &clen);
              } while (csd < 0 && errno == EINTR);
-             if (csd < 0)
-                 log_unixerr("accept", NULL, "socket error: accept failed",
-                             server_conf);
- 	}
  
! 	accept_mutex_off(); /* unlock after "accept" */
  
  	clen = sizeof(sa_server);
! 	if(getsockname(csd, &sa_server, &clen) < 0) {
  	    log_unixerr("getsockname", NULL, NULL, server_conf);
  	    continue;
  	}
  
  	sock_disable_nagle(csd);
  
! 	(void)update_child_status (child_num, SERVER_BUSY_READ, (request_rec*)NULL);
  	conn_io = bcreate(ptrans, B_RDWR);
  	dupped_csd = csd;
  #if defined(NEED_DUPPED_CSD)
--- 1604,1685 ----
  	    exit(0);
  	}
  
! 	(void)update_child_status(child_num, SERVER_READY, (request_rec*)NULL);
  
!         if (listeners == NULL) {
!             FD_ZERO(&listenfds);
!             FD_SET(sd, &listenfds);
!             listenmaxfd = sd;
!         }
  
!         accept_mutex_on();  /* Lock around "accept", if necessary */
  
!         /*
!          * Wait for an acceptable connection to arrive.
!          */
  
!         for (;;) {
!             memcpy(&main_fds, &listenfds, sizeof(fd_set));
  #ifdef SELECT_NEEDS_CAST
!             srv = select(listenmaxfd+1, (int*)&main_fds, NULL, NULL, NULL);
  #else
!             srv = select(listenmaxfd+1, &main_fds, NULL, NULL, NULL);
  #endif
!             sync_scoreboard_image();
!             if (scoreboard_image->global.exit_generation >= generation)
!                 exit(0);
! 
!             if (srv < 0 && errno != EINTR)
!                 log_unixerr("select", "(listen)", NULL, server_conf);
  
!             if (srv <= 0)
!                 continue;
! 
!             if (listeners != NULL) {
!                 for (sd = listenmaxfd; sd >= 0; sd--)
!                     if (FD_ISSET(sd, &main_fds)) break;
!                 if (sd < 0)
!                     continue;
              }
  
              do {
                  clen = sizeof(sa_client);
                  csd  = accept(sd, &sa_client, &clen);
              } while (csd < 0 && errno == EINTR);
  
!             if (csd >= 0)
!                 break;      /* We have a socket ready for reading */
!             else {
! 
! #if defined(EPROTO) && defined(ECONNABORTED)
!               if ((errno != EPROTO) && (errno != ECONNABORTED))
! #elif defined(EPROTO)
!               if (errno != EPROTO)
! #elif defined(ECONNABORTED)
!               if (errno != ECONNABORTED)
! #endif
!                 log_unixerr("accept", "(client socket)", NULL, server_conf);
!             }
!         }
! 
!         accept_mutex_off(); /* unlock after "accept" */
! 
!         /*
!          * We now have a connection, so set it up with the appropriate
!          * socket options, file descriptors, and read/write buffers.
!          */
  
  	clen = sizeof(sa_server);
! 	if (getsockname(csd, &sa_server, &clen) < 0) {
  	    log_unixerr("getsockname", NULL, NULL, server_conf);
  	    continue;
  	}
  
  	sock_disable_nagle(csd);
  
! 	(void)update_child_status(child_num, SERVER_BUSY_READ,
! 	                          (request_rec*)NULL);
! 
  	conn_io = bcreate(ptrans, B_RDWR);
  	dupped_csd = csd;
  #if defined(NEED_DUPPED_CSD)
***************
*** 1686,1734 ****
  				       (struct sockaddr_in *)&sa_server,
  				       child_num);
  
! 	r = read_request (current_conn);
! 	(void)update_child_status (child_num, SERVER_BUSY_WRITE, r);
! 	if (r) process_request (r); /* else premature EOF --- ignore */
! #if defined(STATUS)
!         if (r) increment_counts(child_num,r,1);
! #endif
! 	while (r && current_conn->keepalive) {
! 	    destroy_pool(r->pool);
! 	    (void)update_child_status (child_num, SERVER_BUSY_KEEPALIVE,
! 	     (request_rec*)NULL);
! 	    r = read_request (current_conn);
! 	    (void)update_child_status (child_num, SERVER_BUSY_WRITE, r);
! 	    if (r) process_request (r);
  #if defined(STATUS)
! 	    if (r) increment_counts(child_num,r,0);
  #endif
! 	  sync_scoreboard_image();
! 	  if(scoreboard_image->global.exit_generation >= generation)
! 	      exit(0);
! 	  /*fprintf(stderr,"%d check(3) %d %d\n",getpid(),scoreboard_image->global.exit_generation,generation);*/
  
! 	}
! #if 0	
! 	if (bytes_in_pool (ptrans) > 80000)
! 	    log_printf(r->server,
! 		       "Memory hog alert: allocated %ld bytes for %s",
! 		       bytes_in_pool (ptrans), r->the_request);
! #endif		
! 	bflush(conn_io);
  
  #ifdef NO_LINGCLOSE
! 	bclose(conn_io);	/* just close it */
  #else
!         if (r && !r->connection->aborted) {
! 	    /* if the connection was aborted by a soft_timeout, it has
! 	     * already been shutdown() so we don't need to go through
! 	     * lingering_close
! 	     */
! 	    lingering_close(r);
! 	} else {
! 	    close(conn_io->fd);
! 	}
! #endif	
      }    
  }
  
--- 1695,1752 ----
  				       (struct sockaddr_in *)&sa_server,
  				       child_num);
  
!         /*
!          * Read and process each request found on our connection
!          * until no requests are left or we decide to close.
!          */
! 
!         for (;;) {
!             r = read_request(current_conn);
!             (void)update_child_status(child_num, SERVER_BUSY_WRITE, r);
! 
!             if (r) process_request(r); /* else premature EOF --- ignore */
  #if defined(STATUS)
!             if (r) increment_counts(child_num,r,1);
  #endif
!             if (!r || !current_conn->keepalive)
!                 break;
  
!             destroy_pool(r->pool);
!             (void)update_child_status(child_num, SERVER_BUSY_KEEPALIVE,
!                                       (request_rec*)NULL);
! 
!             sync_scoreboard_image();
!             if (scoreboard_image->global.exit_generation >= generation) {
!                 bclose(conn_io);
!                 exit(0);
!             }
!         }
! 
!         /*
!          * Close the connection, being careful to send out whatever is still
!          * in our buffers.  If possible, try to avoid a hard close until the
!          * client has ACKed our FIN and/or has stopped sending us data.
!          */
  
  #ifdef NO_LINGCLOSE
!         bclose(conn_io);        /* just close it */
  #else
!         if (r &&  r->connection
!               && !r->connection->aborted
!               &&  r->connection->client
!               && (r->connection->client->fd >= 0)) {
! 
!             lingering_close(r);
!         }
!         else {
!             /* if the connection was aborted by a soft_timeout, it has
!              * already been shutdown() so we don't need to go through
!              * lingering_close
!              */
!             bsetflag(conn_io, B_EOUT, 1);
!             bclose(conn_io);
!         }
! #endif
      }    
  }
  
***************
*** 2207,2213 ****
              if (r) process_request (r);
          }
  
- 	bflush(cio);
  	bclose(cio);
      }
      exit (0);
--- 2225,2230 ----

Mime
View raw message