Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix error branches, take 3. #14721

Merged
merged 5 commits into from
Jul 12, 2018
Merged

Conversation

riastradh-brave
Copy link
Contributor

@riastradh-brave riastradh-brave commented Jul 12, 2018

  • Make sure we continue watching for tor restart after an error on the control channel.
  • If we get a watch event while polling, don't cause us to spin polling as fast as the CPU can go until tor is launched.
  • Don't duplicate errors synchronously and asynchronously.
  • Handle asynchronous errors gracefully.
  • Fix some tor redundancy in tor log messages about tor that say tor a few too many times.

fix #14630 for real, maybe?

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

  1. Turn off the network.
  2. Launch Brave.
  3. Open a private tab with Tor.
  4. Wait for it report timeout.
  5. Hit the 'Retry' button as fast as your fingers can manage.
  6. Confirm no stack traces dumped to the console.
  7. After a minute, turn the network back on.
  8. Visit https://check.torproject.org to confirm Tor works.

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

To do this, we separate step of killing the daemon and control channel
from the step of ceasing to watch for updates.  On errors, we want to
kill the daemon and control channel, but continue watching for updates
in case the daemon restarts.

Possible snag: we do not rate-limit failures if they are recursive.
Not currently an issue but we should take care of this in the future.
Don't emit an 'error' event if we did pass them synchronously.

Establish an 'error' handler in filtering.js for asynchronous errors.
@riastradh-brave riastradh-brave force-pushed the riastradh-tor-controlerrorbranches-v3 branch from 99190a7 to 79794d6 Compare July 12, 2018 01:19
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested fairly exhaustively on Windows 10 and these changes worked great for me 👍

@bsclifton bsclifton merged commit 684ed1f into master Jul 12, 2018
@bsclifton bsclifton deleted the riastradh-tor-controlerrorbranches-v3 branch July 12, 2018 05:48
bsclifton added a commit that referenced this pull request Jul 12, 2018
bsclifton added a commit that referenced this pull request Jul 12, 2018
@bsclifton
Copy link
Member

master 684ed1f
0.24.x 764eb6e
0.23.x f456bac

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tor control socket error when network is disconnected
2 participants