[tor-bugs] #31794 [Circumvention/Snowflake]: Errors swallowed

[Tor Bug Tracker & Wiki]

#31794: Errors swallowed
-------------------------------------+--------------------------------
 Reporter:  sah                      |          Owner:  (none)
     Type:  defect                   |         Status:  needs_revision
 Priority:  Medium                   |      Milestone:
Component:  Circumvention/Snowflake  |        Version:
 Severity:  Normal                   |     Resolution:
 Keywords:                           |  Actual Points:
Parent ID:                           |         Points:
 Reviewer:  cohosh                   |        Sponsor:
-------------------------------------+--------------------------------

Comment (by cohosh):

 Replying to [comment:8 sah]:
 > A log.Fatal here would end the goroutine, but it's going to end there
 anyway, but there's no way to tell how far along the main thread has
 progressed since the goroutine was started.
 It's okay to end the goroutine here. The code
 {{{
     for {
         signal := <-sigChan
         log.Println("Received signal:", signal, ". Reloading geoip
 databases.")
         ctx.metrics.LoadGeoipDatabases(geoipDatabase, geoip6Database)
     }
 }}}
 was introduced to allow us to update the geoip database without stopping
 and restarting the broker. If for some reason that update went wrong we
 want to know about it and try again. The broker logs will give information
 about how long the process has been running. We could make the `log.Fatal`
 message reflect that the error occurred due to a SIGHUP signal.

 A slightly better way to handle this would be to log an error and keep
 using the old database. I don't want to do that in this ticket, it would
 require more modifications to the geoip load functions. I'll make a new
 ticket for that.
 >
 > These are linting changes too (Everything I changed is because a linter
 has complained, should I add the linting complaint to the commit message?)
 No need to add all of the linting output. Like I said, just squash all the
 non error linting changes into a single commit and summarize in the commit
 message the types of changes (e.g., removed extraneous else statements,
 changed increment style, etc.) If it looks nicer to use the output from
 the linter, go for it.
 >
 > This does NOT change functionality, you can see that the original code
 instantiates last with a value of time.Now(), then inside the function it
 reassigns time.Now() to last, the only change would be the amount of time
 it took to compute the difference between time.Since(last) and
 time.Second*!LogTimeInterval
 Hmm, nice catch. I'm looking at this part of code closely for the first
 time and it actually looks like a bug was introduced at the time of
 writing. We should just remove
 [https://github.com/cohosh/snowflake/blob/master/client/lib/util.go#L63
 L63] so we have

 {{{
     b.OutEvents++
     if time.Since(last) > time.Second*LogTimeInterval {
         last = time.Now()
         output()
     }
 }}}
 It should match the case below it. Otherwise receiving output bytes is
 resetting our log interval.
 Can you put this fix in a separate commit?

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