[tor-bugs] #2149 [Tor Client]: new 'extra dormant' mode for people who never use their tor

[Tor Bug Tracker & Wiki]

#2149: new 'extra dormant' mode for people who never use their tor
---------------------------------+------------------------------------------
 Reporter:  arma                 |          Owner:                    
     Type:  enhancement          |         Status:  needs_revision    
 Priority:  major                |      Milestone:  Tor: 0.2.4.x-final
Component:  Tor Client           |        Version:                    
 Keywords:  performance scaling  |         Parent:                    
   Points:                       |   Actualpoints:                    
---------------------------------+------------------------------------------

Comment(by sysrqb):

 Replying to [comment:14 nickm]:
 > Thanks for the code!  Just pulled the branch -- The "Using wrong idle
 checker" patch doesn't seem to be there yet?

 That's odd. I'll take a look.
 >
 > Quick review:
 >    * IDLE_TIME_STAGE isn't used.
 I forgot to remove this, fixed.

 >    * stopped_fetching_descriptors isn't documented.
 Missed that. Fixed.

 >    * needs a changelog entry in the changes directory.
 I added one in Using-wrong-idle-checker commit, I wasn't really sure what
 to say, so it'll probably be revised.

 >    * or_options_t is for stuff parsed from torrc and the command line --
 it isn't a catch-all for persistent state.  That would be or_state_t.
 Making this change will also let you back out lots of the
 get_options_mutable() calls, which ought to be a red flag.
 Ah! That makes more sense. I'll make the necessary changes.

 >    * I don't see the point of having DormantStage be global: why can't
 it be a local variable in directory_too_idle_to_fetch_descriptors() ?
 This relates to arma's comment about clients that are consistently idle
 but they don't run continuously for the entire "IDLE_TIME_BEFORE_DORMANT"
 time period. My thinking was that if we add it to the state file then when
 the client starts again it won't necessarily start from time=0. It really
 shouldn't be global, but it needs to be set when the state file is read
 and I wasn't sure how else to accomplish this.

 >    * I don't understand the logic of the changes in conn_read_callback
 and do_main_loop.  In any case, it feels like
 "directory_info_has_arrived()" might be the wrong function to be calling
 here -- after all, you are calling it *not* in response to directory info
 arriving.[*]
 a) Ok, I misunderstood conn_read_callback's use. I'll move the changes.

 b) I seem to have created a bit of duplicate code, I'll fix that. But what
 I was actually trying to do is check that the client is not idle and if it
 is then not call directory_info_has_arrived because it potentially
 downloads new descriptors and such. I toyed with skipping the reload of
 the saved networkstatuses and consensus but decided against it. On
 rethinking this, if we don't call directory_info_has_arrived there's not
 much reason to load the cached info.

 c) I agree. Bad habit. I mostly wanted to make sure I was on the correct
 track with this patch.

 >    * The comment on 59c8df8b59bf04f25 seems wrong; conn_read_callback
 isn't the function that gets called on "an incoming connection" -- it gets
 called every time any socket is readable.  If you want to have something
 happen on incoming connections, that would belong in
 connection_handle_listener_read, I think.
 Same as a) above. I did a lot of manual backtracing and I guess I got a
 little confused along the way.

 >
 > [*] Functions should be called as documented, and not because they have
 some desirable side-effect.  Let's say there is a function called
 computer_is_on_fire() that floods the server room with carbon dioxide. It
 is IMO wrong to call computer_is_on_fire() when the computer is not on
 fire.  If we sometimes want to flood the server room with CO2 when there
 is no fire, then we should make an flood_server_room() function.  That
 makes our code more maintainable and less confusing.

 Thanks for the feedback

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