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

Improve handling when unable to connect to tor #2399

Merged
merged 1 commit into from Feb 14, 2019

Conversation

Projects
None yet
6 participants
@devinbileck
Copy link
Member

devinbileck commented Feb 8, 2019

Issue: If an IOException is raised when attempting to create
tor and the hidden service, the application will just quit without
any indication to the user. One particular scenario where this occurs
is mentioned in #2398.

Cause: There is an explicit statement to exit the application when an
IOException is raised.

Fix: Rather than just exit the application, show an error message
and inform the user what went wrong.

Improve handling when unable to connect to tor
Issue: If an IOException is raised when attempting to create
tor and the hidden service, the application will just quit without
any indication to the user. One particular scenario where this occurs
is mentioned in #2398.

Cause: There is an explicit statement to exit the application when an
IOException is raised.

Fix: Rather than just exit the application, show an error message
and inform the user what went wrong.
@HarryMacfinned

This comment has been minimized.

Copy link

HarryMacfinned commented Feb 9, 2019

Thanks @devinbileck for intercepting this exception early enough.
This should help with the support !

@ManfredKarrer ManfredKarrer requested review from sqrrm , blabno and freimair and removed request for ManfredKarrer , sqrrm and blabno Feb 9, 2019

@freimair
Copy link
Member

freimair left a comment

Has been on my list for quite some time now. Thanks! utAck

@lichuan

This comment has been minimized.

Copy link
Contributor

lichuan commented Feb 10, 2019

Yes, this happened in China, the GFW prevent us connecting TOR, and the Bisq application quit without any dialogs.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 11, 2019

@freimair If there is a tor connection problem the tor settings windows should get displayed. There the user can choose a plugable transport or other custom settings.

@devinbileck

This comment has been minimized.

Copy link
Member Author

devinbileck commented Feb 11, 2019

@ManfredKarrer I agree that it should appear in the case of a connection/communication problem. But in this case it is a setup problem (i.e. cant setup/launch tor in the first place) so there is no need since changing connection settings would have no effect.

@freimair

This comment has been minimized.

Copy link
Member

freimair commented Feb 11, 2019

@ManfredKarrer yes, @devinbileck is right - this code path is mainly used when trying to use system tor. when the connection fails there is nothing we can do.
maybe we want to integrate configuration for system tor into the settings page some time...

System.exit(1);
UserThread.execute(() -> {
setupListeners.stream().forEach(s -> s.onSetupFailed(new RuntimeException(e.getMessage())));
});
} catch (Throwable ignore) {
}

This comment has been minimized.

@devinbileck

devinbileck Feb 11, 2019

Author Member

@freimair Since I removed restartTor(throwable.getMessage()); from onFailure below since I didn't want it to be called after handling IOException above, perhaps I should add it to this ignored exception?

This comment has been minimized.

@freimair

freimair Feb 13, 2019

Member

Maybe we can force onSuccess by return null; after triggering the onSetupFailed and let the onFailure be?

And yes, this class cries for being refactored :)

This comment has been minimized.

@devinbileck

devinbileck Feb 14, 2019

Author Member

In fact we are already doing that. Let me review this again. I should be able to add restartTor back to onFailure.

@devinbileck

This comment has been minimized.

Copy link
Member Author

devinbileck commented Feb 11, 2019

@freimair Integrating configuration for system tor into the settings page sounds like a good idea to me. Perhaps you can open an issue?

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Feb 12, 2019

@devinbileck Is there anything you want to add to this PR, otherwise I would give it an utACK as well and merge it to master for the upcoming release.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Feb 12, 2019

@ripcurlx Please wait with merge for another 1-2 days to get feedback from @freimair .

@ManfredKarrer
Copy link
Member

ManfredKarrer left a comment

ACK

@ManfredKarrer ManfredKarrer merged commit 93558ef into bisq-network:master Feb 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@devinbileck devinbileck deleted the devinbileck:tor-setup-failure branch Feb 14, 2019

@freimair freimair referenced this pull request Feb 28, 2019

Closed

For February 2019 #226

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.