#18809: Handle linked connections better during bootstrap
-------------------------------------------------+-------------------------
Reporter: teor | Owner: arma
Type: defect | Status:
Priority: Medium | needs_revision
Component: Core Tor/Tor | Milestone: Tor:
Severity: Normal | 0.2.8.x-final
Keywords: must-fix-before-028-rc, | Version: Tor:
TorCoreTeam201605, review-group-1 | 0.2.8.1-alpha
Parent ID: | Resolution:
Reviewer: andrea | Actual Points: medium
| Points: medium
| Sponsor:
| SponsorS-can
-------------------------------------------------+-------------------------
Changes (by teor):
* status: needs_review => needs_revision
Comment:
Code review:
ce8266d fix typos/etc before i go nuts on #18809
function rename only
91c5801 avoid following through on a consensus fetch if we have one
already arriving
Suffers from a race condition where two connections could attach at the
same time. But this is unlikely and relatively benign: we'll just fetch
the consensus twice.
59da060 use the new function here too
this is refactoring I should have done
a7665df close other consensus fetches when we get a consensus
There were two race conditions avoided by the previous code:
* the old function closed the connection immediately to avoid wasting
data. But this meant that if a connection was receiving data or undergoing
other operations, there could be errors. I think calling
connection_mark_for_close is better, even if it's less efficient.
* connections can be initiated after
connection_dir_list_by_purpose_and_resource() is called. These connections
are never cleaned up. I think this is fixed by checking when we attach a
stream in 91c5801.
e230e80 get rid of the scattered checks to cancel a consensus fetch
these look good, because we catch it in attach now
bcae392 avoid another redundant check
good, but I wonder if we'll want
networkstatus_consensus_is_downloading_usable_flavor in future. probably
not worth keeping it around
c98fbd4 remove some more unused code
probably not worth keeping it around, but I do wonder if we'll want these
functions
d5a9628 simplify more -- we only call these funcs when bootstrapping
makes sense, but you'd better document that those functions should only
ever be called when bootstrapping
1f72653 fix a bug where relays would use the aggressive client
bootstrapping retry number
I think the correct fix for this is to check `server_mode(options)` in
`consensus_max_download_tries()`, and `return
options->TestingConsensusMaxDownloadTries` if it's true.
Otherwise, this bug is just waiting to happen again.
aa6341d stop looping once we know what the answer will be
early exit FTW
53aaed8 get rid of another no-longer-used function
looks ok
507883e rip out the unit tests for the functions i removed
Please leave the parts of the unit tests that test existing functions, or
remove those functions if they are unused:
* connection_dir_list_by_purpose_and_resource
* connection_dir_count_by_purpose_and_resource
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18809#comment:23>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs
