[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #18873 [Core Tor/Tor]: Refactor circuit_predict_and_launch_new()

#18873: Refactor circuit_predict_and_launch_new()
--------------------------+------------------------------
 Reporter:  asn           |          Owner:
     Type:  defect        |         Status:  new
 Priority:  Low           |      Milestone:  Tor: 0.2.???
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:  refactoring   |  Actual Points:
Parent ID:                |         Points:
 Reviewer:                |        Sponsor:
--------------------------+------------------------------

Comment (by chelseakomlo):

 Ok, here are my changes:

 First, I extracted a useful test helper function. I'm not sure if this is
 the right place to put it though.
 https://github.com/chelseakomlo/tor_patches/commit/7b46a8a45c5a67673a7ee2cd746b58344ba950c7

 Second, I refactored circuit_predict_and_launch_new. This is mainly a
 refactor that improves readability/testability, but I did remove one piece
 of logic which- after writing unit tests to support this- seemed
 unnecessary.
 https://github.com/chelseakomlo/tor_patches/commit/c06005d35d35339e61659cb5c5e43e8bcb60db7d

 The piece of logic that I removed is here:
 https://github.com/chelseakomlo/tor_patches/commit/c06005d35d35339e61659cb5c5e43e8bcb60db7d
 #diff-748f68b44784d7a77ebf42a2db48694eL1025

 As later, we exit if the circuit is not general purpose:
 https://github.com/chelseakomlo/tor_patches/commit/c06005d35d35339e61659cb5c5e43e8bcb60db7d
 #diff-748f68b44784d7a77ebf42a2db48694eL1031

 Let me know if this isn't a good idea and I can add it back in.

 Third, I extracted some magic numbers into named variables. I am not sure
 if the naming for these is the best, please let me know ideas for better
 names.
 https://github.com/chelseakomlo/tor_patches/commit/9ea57c857b3c5939ca3394e017f8de4ea1ac9d08

 A few things that I did not do:
 - I did not add unit tests for needs_hs_client_circuits, as it would
 require mocking the behavior of rep_hist_get_predicted_internal. I could
 definitely do that as part of this patch though.
 - I did not deeply analyze the logic in needs_exit_circuits,
 needs_hs_server_circuits, needs_hs_client_circuits, or
 needs_circuits_for_build_time as I'm still a bit unfamiliar with the
 domain.
 - I did not add integration tests

 One thing I was uncertain about was whether circuit_is_available_for_use
 should be named circuit_is_clean instead.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18873#comment:3>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs