-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
State refactoring #92
Conversation
b88f6da
to
b89a4e3
Compare
While investigating this strangeness I noticed a small bug on this branch that doesn't exist on main. If Tor fails to bootstrap but zipping succeeds then the UI is left in the error state (as expected) and the list of files is not empty (as expected). If I then clear the list of files, the UI doesn't return to the welcome screen (it does on the main branch). The "Try again" button remains visible and it's possible to try again with an empty list of files. Whereas in all other circumstances, clearing the list of files returns me to the welcome screen. You can reproduce this by turning off the phone's internet connection and then waiting for the bootstrapping attempts to time out. |
I added a fix for this now as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look great and the issues I was seeing before have been resolved - nice work! If the phone has no network connection then we correctly end up in the error state after trying all connection methods.
There's still one issue, though, which is only noticeable when it takes more than 2 minutes to zip the files.
In my test run, Tor bootstrapped quickly before the 1GB file had been zipped, so we quickly reached the state Starting(zipPercent=0, torPercent=95)
. Then we stayed in that state for the next 2 minutes while the file was being zipped. Unfortunately TorManager thought that staying in the Starting state for 2 minutes without making progress meant that Tor was failing to bootstrap, even though it had already bootstrapped. So TorManager tried to fetch bridges from Moat and then used the built-in bridges. Fortunately the built-in bridges worked, so when the zipping eventually finished we could publish the hidden service and reach torProgress=100
and everything was cool. But we shouldn't have used bridges.
I guess the solution might be to add a Started
state to indicate that Tor has bootstrapped but we haven't yet published the hidden service?
and let user try again
We weren't properly showing the empty state when the last file got removed.
and add missing NotificationManager calls.
to benefit from its exhaustive nature.
as TorWrapper starts with a fresh config each time and doesn't use bridges by default
Also write first tests for TorManager
This is because previously we only considered uploading the HS desc as started. However, this only happens after zipping files. If we zipping takes a long time, Tor would think it hasn't started and tries circumvention methods. Using a new Started state prevents this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view this is ready to go. Nice work!
Opening this draft PR so we can discuss the work in progress.