[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 sah):

 Thanks for your feedback.

 WRT informative error messages, I wholeheartedly agree, they'll get fixed
 later on today.

 WRT "As mentioned above,
 [https://github.com/shaneHowearth/snowflake/blob/master/broker/broker.go#L384
 ​this] geoip reload should log a Fatal error. See the call to the same
 function earlier in the code to see how to do it." Because this failure is
 inside an anonymous function inside a goroutine I actually feel that the
 whole error handling inside there needs attention. (The earlier call is in
 the 'main' thread). A log.Fatal here would end the goroutine, but it's
 going to end there anyway, and has no effect on the functionality of the
 code in either case.

 WRT "While I understand the desire to do so,
 [https://github.com/shaneHowearth/snowflake/commit/77e1ab458a6a742246cf4f42d7d54b2ea77c2814
 #diff-c8ef7ba143d251e41b143b2ab02f3733L136 ​this] shouldn't really be in
 this patch set since it has nothing to do with the ticket. Same with
 [https://github.com/shaneHowearth/snowflake/commit/77e1ab458a6a742246cf4f42d7d54b2ea77c2814
 #diff-345294112d730f1e04b863fb1e5c0981L184 ​this change]. There are more
 but I'm not going to list them all now. What I'm going to recommend is to
 squash all of the error handling changes into a single commit, and then
 have a separate commit for other linting changes." These are linting
 changes too (Everything I changed is because a linter has complained,
 should I add the linting complaint to the commit message?)


 WRT "Why did you make the change
 [https://github.com/shaneHowearth/snowflake/commit/9d6fabee2c6596fcf72518f31188219b66576525
 #diff-9d5c4d1c03bbea44d537ac915acf610aL65 ​here]? It changes the
 functionality of the code and should be reverted." 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

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