Add cleanup tor files button to tor network settings #1301

ManfredKarrer · 2018-01-31T05:27:43Z

Update (by @cbeams):

This PR fixes #1300. The following is from @ManfredKarrer's original comment there:

I observed an issue with one seed node and after deleting the whole tor directory in the app dir folder (except the hidden service folder!) it seems to have resolved the issue. I assume some files there get invalidated and cause issues.
I will add a button in settings/network to do that deletion in a safe way (not deleting the hidden service folder) to give users an easy option to fix similar issues.

@ManfredKarrer's original text in this PR:

Could you also review the deleteDirectory method in FileUtils. We have to be sure to not delete the hidden service folder. As far I tested it it worked...

   public static void deleteDirectory(File file, File exclude) throws IOException {
        boolean excludeFileFound = false;
        if (file.isDirectory()) {
            File[] files = file.listFiles();
            if (files != null)
                for (File f : files) {
                    if (!excludeFileFound)
                        excludeFileFound = f.equals(exclude);
                    if (!f.equals(exclude))
                        deleteDirectory(f);
                }
        }
        // Finally delete main file/dir if exclude file was not found in directory
        if (!excludeFileFound && !file.equals(exclude))
            deleteFileIfExists(file);
    }

cbeams · 2018-01-31T13:45:54Z

I'm reviewing this now, @ManfredKarrer.

cbeams

NACK. See comments for details. In short:

A couple typos in user-facing messages that need to be fixed
A couple style nits around ctor param stacking

I tested the new functionality and confirmed that it left my tor/hiddenservice dir intact as expected.

As a more general note, it seems that we should have a dedicated Tor network view, as opposed to this sort of overloaded use of a dialog. Not saying that needs to be fixed in order to merge this, just mentioning it. Perhaps you think the same, and this is just more expedient right now, I don't know.

Also, if people are having trouble connecting in the first place, and are never getting past the splash screen, how are they going to get to these network settings at all in order to delete the Tor directory? I must be missing something.

And another thing: I have no idea why deleting the files in this directory would make a difference at all. Based on your original comments in the description of this PR, it sounds like this change is all based on a pragmatic observation that when you delete this directory, everything seems to start working. Not particularly satisfying, but I can't really argue with it either. Would be great to know what is actually making a difference here though.

cbeams · 2018-01-31T14:49:36Z

common/src/main/resources/i18n/displayStrings.properties

-torNetworkSettingWindow.info=If Tor is blocked by your internet provider or in your country you can try to use Tor bridges.\n\
+torNetworkSettingWindow.deleteFiles.header=Connection problems?
+torNetworkSettingWindow.deleteFiles.info=If you have repeated connection problems at start up, deleting outdated Tor files might help. To do that click the button below and restart afterwards.
+torNetworkSettingWindow.deleteFiles.button=Deleting outdated Tor files and shut down


s/Deleting/Delete/

cbeams · 2018-01-31T14:50:00Z

common/src/main/resources/i18n/displayStrings.properties

+torNetworkSettingWindow.deleteFiles.header=Connection problems?
+torNetworkSettingWindow.deleteFiles.info=If you have repeated connection problems at start up, deleting outdated Tor files might help. To do that click the button below and restart afterwards.
+torNetworkSettingWindow.deleteFiles.button=Deleting outdated Tor files and shut down
+torNetworkSettingWindow.deleteFiles.success=Deleting outdated Tor files wa successful. Please restart.


cbeams · 2018-01-31T15:02:12Z

gui/src/main/java/io/bisq/gui/main/settings/network/NetworkSettingsView.java

+                               BisqEnvironment bisqEnvironment,
+                               TorNetworkSettingsWindow torNetworkSettingsWindow,
+                               Clock clock,
+                               BSFormatter formatter) {


I don't think we want to move to one-parameter per line parameter stacking like this. Creates a lot of vertical noise, and constructors with a zillion parameters are usually a signal of something needing to be refactored anyway. I'd prefer that we keep parameters aligned with the opening paren, but not stack one per line. Just go to the 120 margin, and create a new line as necessary.

Re typos:
Fixed in next commit.

Re Popup:
The popup is used when Tor cannot start up after 4 minutes at splash screens. So that is a reason why a dedicated Tor view will not work here. It is also considered as "exceptional" screen which should not be needed for normal use case.

Re why it changes anything:
I agree it is not satisfying not knowing more details. Will be a task for our new P2P network dev. There are some consensus files which might get outdated. I just have seen it yesterday with a seed node which kept failing after repeated restarts and after deleting the Tor dir it was fine.

Re ctor params:
I personally prefer it for readability to have one per line if its more then 3 or 4 params...
But not super strong opinion here. we can discuss... I prefer to keep it atm as I usually use it that way in most of the newer code parts.

Re deleteDirectory:
Do you see any risks in that method?

Makes sense, re popup. I didn't realize that before. Thanks.

On deleteDirectory I didn't analyze it line by line, but gave it a good look and then tested it end-to-end on my machine. This is the only place it's used in the codebase, so I'd say the risk is contained for now. Would be good to have tests around it for sure, but I'm ok with it as-is now.

cbeams · 2018-01-31T15:02:58Z

gui/src/main/java/io/bisq/gui/main/MainViewModel.java

@@ -219,7 +219,7 @@ public MainViewModel(WalletsManager walletsManager, WalletsSetup walletsSetup,
                         DaoManager daoManager, EncryptionService encryptionService,
                         KeyRing keyRing, BisqEnvironment bisqEnvironment, FailedTradesManager failedTradesManager,
                         ClosedTradableManager closedTradableManager, AccountAgeWitnessService accountAgeWitnessService,
-                         BSFormatter formatter) {
+                         TorNetworkSettingsWindow torNetworkSettingsWindow, BSFormatter formatter) {


This is a good example of adding a constructor parameter (see other comments)

cbeams · 2018-01-31T15:03:16Z

gui/src/main/java/io/bisq/gui/main/MainView.java

+    public MainView(MainViewModel model,
+                    CachingViewLoader viewLoader,
+                    Navigation navigation,
+                    Transitions transitions,


Avoid one per line ctor parameter stacking (see other, more detailed comment)

cbeams

ACK.

cbeams · 2018-01-31T17:20:28Z

gui/src/main/java/io/bisq/gui/main/settings/network/NetworkSettingsView.java

+                               BisqEnvironment bisqEnvironment,
+                               TorNetworkSettingsWindow torNetworkSettingsWindow,
+                               Clock clock,
+                               BSFormatter formatter) {


Makes sense, re popup. I didn't realize that before. Thanks.

On deleteDirectory I didn't analyze it line by line, but gave it a good look and then tested it end-to-end on my machine. This is the only place it's used in the codebase, so I'd say the risk is contained for now. Would be good to have tests around it for sure, but I'm ok with it as-is now.

Add cleanup tor files button to tor network settings

64ba300

ManfredKarrer added this to the v0.6.5 milestone Jan 31, 2018

ManfredKarrer self-assigned this Jan 31, 2018

ManfredKarrer requested a review from cbeams January 31, 2018 05:27

ManfredKarrer mentioned this pull request Jan 31, 2018

Add button for deleting Tor directory except hidden service folder #1300

Closed

cbeams self-assigned this Jan 31, 2018

cbeams suggested changes Jan 31, 2018

View reviewed changes

Fix typos

eeeabaf

cbeams approved these changes Jan 31, 2018

View reviewed changes

cbeams merged commit 7beba50 into master Jan 31, 2018

cbeams deleted the option-to-delete-tor-files branch January 31, 2018 17:27

cbeams mentioned this pull request Jan 31, 2018

Investigate Tor network issues #1241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cleanup tor files button to tor network settings #1301

Add cleanup tor files button to tor network settings #1301

ManfredKarrer commented Jan 31, 2018 •

edited by cbeams

Loading

cbeams commented Jan 31, 2018

cbeams left a comment

cbeams Jan 31, 2018

cbeams Jan 31, 2018

cbeams Jan 31, 2018

ManfredKarrer Jan 31, 2018

cbeams Jan 31, 2018

cbeams Jan 31, 2018

cbeams Jan 31, 2018

cbeams left a comment

cbeams Jan 31, 2018

Add cleanup tor files button to tor network settings #1301

Add cleanup tor files button to tor network settings #1301

Conversation

ManfredKarrer commented Jan 31, 2018 • edited by cbeams Loading

cbeams commented Jan 31, 2018

cbeams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManfredKarrer commented Jan 31, 2018 •

edited by cbeams

Loading