-
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
Stop sharing when Tor stops during startup #52
Stop sharing when Tor stops during startup #52
Conversation
5710dcf
to
0c397aa
Compare
// release persistable Uri permissions again | ||
shareState.value.files.forEach { file -> | ||
file.releaseUriPermission() | ||
LOG.error("Unhandled state: ↑") |
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.
This entire shareState
method is quite a beast and does not handle all permutations of states. If there's any suggestions how to reconcile the states of the various sub-systems in a better way, please let me know!
ensureActive() | ||
// When the current scope gets cancelled, the async routine gets cancelled 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.
Does the same apply to torManager.start() below? What are the implications of cancelling that call?
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.
Yes, there a JobCancellationException
gets thrown which we catch and call stop()
which stops Tor again.
@Suppress("BlockingMethodInNonBlockingContext") | ||
private suspend fun startSharing() { | ||
if (startSharingJob?.isActive == true) { | ||
// TODO check if this always works as expected | ||
startSharingJob?.cancelAndJoin() | ||
} | ||
shouldStop.value = false | ||
// Attention: We'll launch sharing in Global scope, so it survives ViewModel death |
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.
Stale comment?
If we're no longer using the global scope then does the job still survive the death of the view model?
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.
Ahh good catch. Through some corners this actually inherits the ViewModelScope, so this needs to go back to GlobalScope.
I rebased this branch on master and left it running over the weekend. Tor started successfully, which hasn't happened on master in many attempts. Shortly after the onion address appeared I was able to open the download page in Tor Browser. I didn't download the file. After about 24 hours OnionShare was still running and the onion address was still shown in the UI, but the hidden service was no longer reachable in Tor Browser. The same was true this morning (about 60 hours after starting OnionShare). When I tapped the stop button in OnionShare it got stuck in the "stopping" state, with the notification still showing "Currently sharing files". It stayed in this state for several hours. Exiting the app via the back button and relaunching didn't make a difference. Eventually I had to force-stop the app. Unfortunately since the app was running for so long, all the useful information had expired from the system log. I'll try to reproduce this without leaving the app running for so long. |
3535334
to
8c1e488
Compare
Before, ShareService was binding to OnionService which prevented TorService's stopSelf() method from working.
Uses DiscardPK flag
when starting takes a long time, it can be useful to show that files are not yet published.
8c1e488
to
3573ca1
Compare
3573ca1
to
8f0af15
Compare
Each manager is now publishing its own state flow and the share manager combines all three into one sharing state flow, so it can update that state even after sharing is complete and the onionshare was published.
Related to #12