[or-cvs] holding until flush was borked

[arma@seul.org (Roger Dingledine)]

Update of /home/or/cvsroot/src/or
In directory moria.mit.edu:/home2/arma/work/onion/cvs/src/or

Modified Files:
	connection.c connection_edge.c main.c 
Log Message:
holding until flush was borked
we were never writing anything when hold_open_until_flushed was set,
since conn_write returns early if marked_for_conn is set.

seems a bit better now.


Index: connection.c
===================================================================
RCS file: /home/or/cvsroot/src/or/connection.c,v
retrieving revision 1.173
retrieving revision 1.174
diff -u -d -r1.173 -r1.174
--- connection.c	3 Mar 2004 07:26:34 -0000	1.173
+++ connection.c	3 Mar 2004 08:46:18 -0000	1.174
@@ -226,6 +226,7 @@
     if (conn->hold_open_until_flushed) {
       assert(conn->marked_for_close);
       if (now - conn->timestamp_lastwritten >= 15) {
+        log_fn(LOG_WARN,"Giving up on marked_for_close conn that's been flushing for 15s (fd %d, type %d, state %d).", conn->s, conn->type, conn->state);
         conn->hold_open_until_flushed = 0;
       }
     }

Index: connection_edge.c
===================================================================
RCS file: /home/or/cvsroot/src/or/connection_edge.c,v
retrieving revision 1.111
retrieving revision 1.112
diff -u -d -r1.111 -r1.112
--- connection_edge.c	3 Mar 2004 07:24:53 -0000	1.111
+++ connection_edge.c	3 Mar 2004 08:46:18 -0000	1.112
@@ -58,6 +58,8 @@
     /* eof reached, kill it. */
     log_fn(LOG_INFO,"conn (fd %d) reached eof. Closing.", conn->s);
     connection_mark_for_close(conn, END_STREAM_REASON_DONE);
+    conn->hold_open_until_flushed = 1; /* just because we shouldn't read
+                                          doesn't mean we shouldn't write */
     return 0;
 #endif
   }

Index: main.c
===================================================================
RCS file: /home/or/cvsroot/src/or/main.c,v
retrieving revision 1.190
retrieving revision 1.191
diff -u -d -r1.190 -r1.191
--- main.c	3 Mar 2004 07:26:58 -0000	1.190
+++ main.c	3 Mar 2004 08:46:18 -0000	1.191
@@ -205,9 +205,9 @@
     return; /* this conn doesn't want to write */
 
   conn = connection_array[i];
+  log_fn(LOG_DEBUG,"socket %d wants to write.",conn->s);
   if (conn->marked_for_close)
     return;
-  log_fn(LOG_DEBUG,"socket %d wants to write.",conn->s);
 
   assert_connection_ok(conn, time(NULL));
 
@@ -225,44 +225,49 @@
 
 static void conn_close_if_marked(int i) {
   connection_t *conn;
+  int retval;
 
   conn = connection_array[i];
   assert_connection_ok(conn, time(NULL));
-  if(conn->marked_for_close) {
-    if (conn->hold_open_until_flushed && conn->s >= 0 &&
-        connection_wants_to_flush(conn))
-      return;
+  if(!conn->marked_for_close)
+    return; /* nothing to see here, move along */
 
-    log_fn(LOG_INFO,"Cleaning up connection (fd %d).",conn->s);
-    if(conn->s >= 0 && connection_wants_to_flush(conn)) {
-      /* -1 means it's an incomplete edge connection, or that the socket
-       * has already been closed as unflushable. */
-      /* FIXME there's got to be a better way to check for this -- and make other checks? */
+  log_fn(LOG_INFO,"Cleaning up connection (fd %d).",conn->s);
+  if(conn->s >= 0 && connection_wants_to_flush(conn)) {
+    /* -1 means it's an incomplete edge connection, or that the socket
+     * has already been closed as unflushable. */
+    if(!conn->hold_open_until_flushed)
       log_fn(LOG_WARN,
-             "Conn (fd %d, type %d, state %d) marked for close, but wants to flush %d bytes. "
-             "Marked at %s:%d",
-             conn->s, conn->type, conn->state,
-             conn->outbuf_flushlen, conn->marked_for_close_file, conn->marked_for_close);
-      /* XXX change the above to 'warn', and go through and fix all the complaints */
-      if(connection_speaks_cells(conn)) {
-        if(conn->state == OR_CONN_STATE_OPEN) {
-          flush_buf_tls(conn->tls, conn->outbuf, &conn->outbuf_flushlen);
-        }
-      } else {
-        flush_buf(conn->s, conn->outbuf, &conn->outbuf_flushlen);
-      }
-      if(connection_wants_to_flush(conn) && buf_datalen(conn->outbuf)) {
-        log_fn(LOG_WARN,"Conn (fd %d) still wants to flush. Losing %d bytes!",
-               conn->s, (int)buf_datalen(conn->outbuf));
-      }
+        "Conn (fd %d, type %d, state %d) marked, but wants to flush %d bytes. "
+        "(Marked at %s:%d)",
+        conn->s, conn->type, conn->state,
+        conn->outbuf_flushlen, conn->marked_for_close_file, conn->marked_for_close);
+    if(connection_speaks_cells(conn)) {
+      if(conn->state == OR_CONN_STATE_OPEN) {
+        retval = flush_buf_tls(conn->tls, conn->outbuf, &conn->outbuf_flushlen);
+        /* XXX actually, some non-zero results are maybe ok. which ones? */
+      } else
+        retval = -1;
+    } else {
+      retval = flush_buf(conn->s, conn->outbuf, &conn->outbuf_flushlen);
     }
-    connection_remove(conn);
-    connection_free(conn);
-    if(i<nfds) { /* we just replaced the one at i with a new one.
-                    process it too. */
-      conn_close_if_marked(i);
+    if(retval == 0 &&
+       conn->hold_open_until_flushed && connection_wants_to_flush(conn)) {
+      log_fn(LOG_INFO,"Holding conn (fd %d) open for more flushing.",conn->s);
+      /* XXX should we reset timestamp_lastwritten here? */
+      return;
+    }
+    if(connection_wants_to_flush(conn)) {
+      log_fn(LOG_WARN,"Conn (fd %d) still wants to flush. Losing %d bytes!",
+             conn->s, (int)buf_datalen(conn->outbuf));
     }
   }
+  connection_remove(conn);
+  connection_free(conn);
+  if(i<nfds) { /* we just replaced the one at i with a new one.
+                  process it too. */
+    conn_close_if_marked(i);
+  }
 }
 
 /* Perform regular maintenance tasks for a single connection.  This