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.


svn:r1214

3 files changed, 40 insertions, 32 deletions

diff --git a/src/or/connection.c b/src/or/connection.c
index 1ec9df3006..c20a8b0418 100644
--- a/src/or/connection.c
+++ b/src/or/connection.c
@@ -226,6 +226,7 @@ void connection_expire_held_open(void)
     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;
       }
     }
diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 38aedb385f..848003012e 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -58,6 +58,8 @@ int connection_edge_process_inbuf(connection_t *conn) {
     /* 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
   }
diff --git a/src/or/main.c b/src/or/main.c
index 139882e352..6b1f226412 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -205,9 +205,9 @@ static void conn_write(int i) {
     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,43 +225,48 @@ static void conn_write(int i) {
 
 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;
-
-    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? */
+  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. */
+    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);
   }
 }