Skip to content
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

Merged
merged 17 commits into from
Aug 2, 2023
Merged

State refactoring #92

merged 17 commits into from
Aug 2, 2023

Conversation

akwizgran
Copy link
Collaborator

Opening this draft PR so we can discuss the work in progress.

@grote grote force-pushed the states branch 3 times, most recently from b88f6da to b89a4e3 Compare June 9, 2023 19:13
@akwizgran
Copy link
Collaborator Author

akwizgran commented Jul 21, 2023

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.

@grote
Copy link
Collaborator

grote commented Aug 1, 2023

If I then clear the list of files, the UI doesn't return to the welcome screen (it does on the main branch).

I added a fix for this now as well.

Copy link
Collaborator Author

@akwizgran akwizgran left a 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?

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.
@akwizgran akwizgran marked this pull request as ready for review August 2, 2023 10:47
Copy link
Collaborator Author

@akwizgran akwizgran left a 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!

@akwizgran akwizgran changed the title Draft: State refactoring State refactoring Aug 2, 2023
@grote grote merged commit 4d0604e into onionshare:main Aug 2, 2023
1 check passed
@grote grote deleted the states branch August 2, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants