[tor-bugs] #23709 [Core Tor/Tor]: channel: `outgoing_queue` and `incoming_queue` are always empty

[Tor Bug Tracker & Wiki]

#23709: channel: `outgoing_queue` and `incoming_queue` are always empty
------------------------------+--------------------------------
     Reporter:  dgoulet       |      Owner:  (none)
         Type:  defect        |     Status:  new
     Priority:  High          |  Milestone:  Tor: 0.3.3.x-final
    Component:  Core Tor/Tor  |    Version:
     Severity:  Normal        |   Keywords:  tor-sched
Actual Points:                |  Parent ID:
       Points:                |   Reviewer:
      Sponsor:                |
------------------------------+--------------------------------
 1. Let's start with the `incoming_queue` of a channel_t:

 The `channel_queue_(var)cell()` function is the one taking a single cell
 and putting it in the in queue.

  If the queue is empty, it processes the cell immediately.
  If the queue is NOT empty, it puts the cell in the queue and processes
 the cell immediately if we have cell handlers which is currently always
 the case.

 The "process cell" function is in charge of removing the cell from the
 queue. So I think you can clearly see the problem with this code flow ;).

 2. Now `outgoing_queue` of a channel_t:

 Inserting a cell in that queue is with `channel_write_cell_queue_entry()`
 which does it IFF the `outgoing_queue` is not empty and channel is open.
 If the queue is empty, the cell is processed immediately with the
 `write_cell` handler.

 If the cell was for whatever reason not able to be sent by the write
 handler, the cell is then put in the `outgoing_queue`. However, the only
 possible handler we have right now is `channel_tls_write_cell_method()`
 (and packed/var friends) that always return 1 if the `tlschan->conn`
 exists. Empirical test shows that it is really ALWAYS the case that a conn
 exists on the tls channel.

 ---

 Bottom line, those queues are as good as local to a function but we do
 consider them everywhere in the code such as in
 `channel_flush_some_cells()` or `channel_more_to_flush()`.

 We should either get rid of them or use them properly. For KIST, this is a
 big deal though to have them work properly because once data goes from a
 cmux queue to the outbuf of the connection, libevent will kick in to
 send() the data from the outbuf of a connection. KIST needs to have this
 steps where once it puts the data in the outbuf it then knows it has to
 write to kernel. If it can NOT write to kernel, the data needs to stay in
 the out queue else libevent will write the data from the outbuf to kernel
 without the scheduler knowing about it.

 But because those queues are basically not usable outside their functions,
 everytime we queue a cell, it is put immediately in the outbuf.
 Fortunately, KIST does control a bit of it by doing the flush from the
 cmux to the outbuf (`channel_flush_some_cells()`) and then tries to write
 to kernel what has been flushed. But is seems that tor can put cells in
 the outbuf in other places which is not ideal...

 If we can make the scheduler the *only* one capable of flushing the cmux
 to the out buf, we could get rid of those queues and only use the cmux
 queue. The scheduler does NOT care about the incoming_queue at all.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/23709>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online