Defer reporting bootstrap progress (ticket 27169) #315
Conversation
Simplify control_event_bootstrap() by making it return void again. It is currently a fairly complicated function, and it's made more complicated by returning an int to signal whether it logged at NOTICE or INFO. The callers conditionally log messages at level NOTICE based on this return value. Change the callers to unconditionally log their verbose human-readable messages at level INFO to keep NOTICE logs less cluttered. This partially reverts the changes of #14950.
Move the mostly-invariant part of control_event_boostrap() into a helper control_event_bootstrap_core(). The helper doesn't modify any state beyond doing logging and control port notifications.
Eliminate a few conditional expressions in control_event_bootstrap_core() by overwriting the status parameter.
Track bootstrap phase (enumerated by bootstrap_status_t) independently from the bootstrap progress (which can represent intermediate progress). This allows control_event_bootstrap_problem() to avoid doing a linear search through the bootstrap progress space to find the current bootstrap phase.
Existing cached directory information can cause misleadingly high bootstrap percentages. To improve user experience, defer reporting of directory information progress until at least one connection has succeeded to a relay or bridge. Closes ticket 27169.
tlyu
changed the title
| if (status > bootstrap_percent || | ||
| (progress && progress > bootstrap_percent)) { | ||
| int loglevel = LOG_NOTICE; | ||
| bootstrap_status_to_string(status, &tag, &summary); |
By removing this, I believe tag and summary will be undefined if they hit the snprintf below.
This is a minor mistake I made when rewriting history. 15c24d6 restores the call to bootstrap_status_to_string(), so the right thing happens if all the patches are applied. I can fix this one commit by editing the branch if you like.
| while (status>=0 && bootstrap_status_to_string(status, &tag, &summary) < 0) | ||
| status--; /* find a recognized status string based on current progress */ | ||
| status = bootstrap_percent; /* set status back to the actual number */ | ||
| tor_assert(bootstrap_status_to_string(bootstrap_phase, &tag, &summary) == 0); |
After this removal, is status correct? It points to bootstrap_percent from the initial assignment and it is now never changed. Is this accurate since we seems to rely on the bootstrap_phase but still the status value is used in the log after ("Problem bootstrapping....") ?
Yes, it should be correct. status now never gets modified by the loop, so it remains set to bootstrap_percent by its initializer. We use bootstrap_phase to get the milestone tag and summary, but we report bootstrap_percent (or equivalently, status) as the "percent" progress in the log message.
If it helps, I can eliminate the status variable and use bootstrap_phase and bootstrap_percent for clarity.
| @@ -705,6 +705,7 @@ connection_or_finished_connecting(or_connection_t *or_conn) | |||
| log_debug(LD_HANDSHAKE,"OR connect() to router at %s:%u finished.", | |||
| conn->address,conn->port); | |||
| control_event_bootstrap(BOOTSTRAP_STATUS_HANDSHAKE, 0); | |||
| control_event_boot_first_orconn(); | |||
So the bootstrap process starts with connecting to a directory server (5%). Then, right here at this point, the boostrap will change to Finishing handshake with directory server (10%). With this addition, we'll flag that we have a "first connect() to a OR".
Which means that we can't go pass 10% in the first place if we haven't at least connected to our directory server right? Which makes me wonder if this will really delay any directory information bootstrap progress if our first connect to an OR conn is always at the 10% mark? Won't we be stuck at 5% if we are unable to connect to an OR? (in this case at 5%, the directory server).
I think being stuck at 5% or 10% gives the user a clearer indication of what's wrong than being stuck at 80%.
Which means that we can't go pass 10% in the first place if we haven't at least connected to our directory server right?
Empirically, if there is enough cached directory info, bootstrap will jump from 0% to 80% without necessarily being able to connect to any relay or bridge. I can paste example logs of "warm cache" cases both before and after these patches, if that helps. (probably to the Trac ticket though)
Empirically, if there is enough cached directory info, bootstrap will jump from 0% to 80% without necessarily being able to connect to any relay or bridge. I can paste example logs of "warm cache" cases both before and after these patches, if that helps. (probably to the Trac ticket though)
Oh ok I see the jump from 0 to 80 but stuck at the connecting to OR step which is not fun for the user.
No description provided.
The text was updated successfully, but these errors were encountered: