[or-cvs] [tor/maint-0.2.2 4/5] Revise comment on 2210 a little; clean up n_streams/num_streams confusion

[nickm@xxxxxxxxxxxxxx]

Author: Nick Mathewson <nickm@xxxxxxxxxxxxxx>
Date: Mon, 29 Nov 2010 15:59:59 -0500
Subject: Revise comment on 2210 a little; clean up n_streams/num_streams confusion
Commit: 25b0fd88689becd5c10d871d10099f01411a5571

Also add a changes file
---
 changes/bug2210 |    5 +++++
 src/or/relay.c  |   54 +++++++++++++++++++++++++++---------------------------
 2 files changed, 32 insertions(+), 27 deletions(-)
 create mode 100644 changes/bug2210

diff --git a/changes/bug2210 b/changes/bug2210
new file mode 100644
index 0000000..739a8a9
--- /dev/null
+++ b/changes/bug2210
@@ -0,0 +1,5 @@
+  o ?? bugfixes:
+    - Fix a bug that would cause older streams on a given circuit to
+      get preference when reading bytes from the network.  Fixes bug
+      2210.  Fix by Mashael AlSabah.  This bug was introduced before
+      the first Tor release, in svn revision r152.
diff --git a/src/or/relay.c b/src/or/relay.c
index c64afe2..a65280e 100644
--- a/src/or/relay.c
+++ b/src/or/relay.c
@@ -1478,11 +1478,10 @@ circuit_resume_edge_reading_helper(edge_connection_t *first_conn,
                                    crypt_path_t *layer_hint)
 {
   edge_connection_t *conn;
-  int n_streams, n_streams_left;
+  int n_packaging_streams, n_streams_left;
   int packaged_this_round;
   int cells_on_queue;
   int cells_per_conn;
-  int num_streams = 0;
   edge_connection_t *chosen_stream = NULL;
 
   /* How many cells do we have space for?  It will be the minimum of
@@ -1498,30 +1497,31 @@ circuit_resume_edge_reading_helper(edge_connection_t *first_conn,
   if (CELL_QUEUE_HIGHWATER_SIZE - cells_on_queue < max_to_package)
     max_to_package = CELL_QUEUE_HIGHWATER_SIZE - cells_on_queue;
 
-  /* Count how many non-marked streams there are that have anything on
-   * their inbuf, and enable reading on all of the connections. */
-  n_streams = 0;
-  /* This used to start listening on the streams in the order they
-   * appeared in the linked list.  That leads to starvation in the
-   * event that, for example, our circuit window is almost full, and
-   * there are lots of streams.  Then the first few streams will have
-   * data read from them, and then the window will close again.  When
-   * it reopens, we would enable reading from the beginning of the list
-   * again.  Instead, we just pick a random stream on the list, and
-   * enable reading for streams starting at that point (and wrapping
-   * around as if the list were circular).  It would probably be better
-   * to actually remember which streams we've serviced in the past, but
-   * this is simple and effective. */
+  /* Once we used to start listening on the streams in the order they
+   * appeared in the linked list.  That leads to starvation on the
+   * streams that appeared later on the list, since the first streams
+   * would always get to read first.  Instead, we just pick a random
+   * stream on the list, and enable reading for streams starting at that
+   * point (and wrapping around as if the list were circular).  It would
+   * probably be better to actually remember which streams we've
+   * serviced in the past, but this is simple and effective. */
 
   /* Select a stream uniformly at random from the linked list.  We
    * don't need cryptographic randomness here. */
-  for (conn = first_conn; conn; conn = conn->next_stream) {
-    num_streams++;
-    if ((tor_weak_random() % num_streams)==0)
-      chosen_stream = conn;
-    /* Invariant: chosen_stream has been chosen uniformly at random from among
-     * the first num_streams streams on first_conn. */
+  {
+    int num_streams = 0;
+    for (conn = first_conn; conn; conn = conn->next_stream) {
+      num_streams++;
+      if ((tor_weak_random() % num_streams)==0)
+        chosen_stream = conn;
+      /* Invariant: chosen_stream has been chosen uniformly at random from
+       * among the first num_streams streams on first_conn. */
+    }
   }
+
+  /* Count how many non-marked streams there are that have anything on
+   * their inbuf, and enable reading on all of the connections. */
+  n_packaging_streams = 0;
   /* Activate reading starting from the chosen stream */
   for (conn=chosen_stream; conn; conn = conn->next_stream) {
     /* Start reading for the streams starting from here */
@@ -1531,7 +1531,7 @@ circuit_resume_edge_reading_helper(edge_connection_t *first_conn,
       connection_start_reading(TO_CONN(conn));
 
       if (buf_datalen(conn->_base.inbuf) > 0)
-        ++n_streams;
+        ++n_packaging_streams;
     }
   }
   /* Go back and do the ones we skipped, circular-style */
@@ -1542,16 +1542,16 @@ circuit_resume_edge_reading_helper(edge_connection_t *first_conn,
       connection_start_reading(TO_CONN(conn));
 
       if (buf_datalen(conn->_base.inbuf) > 0)
-        ++n_streams;
+        ++n_packaging_streams;
     }
   }
 
-  if (n_streams == 0) /* avoid divide-by-zero */
+  if (n_packaging_streams == 0) /* avoid divide-by-zero */
     return 0;
 
  again:
 
-  cells_per_conn = CEIL_DIV(max_to_package, n_streams);
+  cells_per_conn = CEIL_DIV(max_to_package, n_packaging_streams);
 
   packaged_this_round = 0;
   n_streams_left = 0;
@@ -1599,7 +1599,7 @@ circuit_resume_edge_reading_helper(edge_connection_t *first_conn,
   if (packaged_this_round && packaged_this_round < max_to_package &&
       n_streams_left) {
     max_to_package -= packaged_this_round;
-    n_streams = n_streams_left;
+    n_packaging_streams = n_streams_left;
     goto again;
   }
 
-- 
1.7.1