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

Avoid startup failure when appDataDir is a symbolic link #4002

Merged
merged 3 commits into from Feb 25, 2020

Conversation

@cbeams
Copy link
Member

cbeams commented Feb 24, 2020

This PR fixes #3998 as explained in the commit comment for cbe6f19.

@cbeams cbeams self-assigned this Feb 24, 2020
Previously, Config#mkdir and #mkAppDir threw ConfigException if any
underlying IOException was thrown from Files#createDirectory or
Files#createDirectories respectively. This resulted in only a simple
error message being reported at the command line and the details of the
underlying exception's stack trace were lost. This change wraps and
rethrows the underlying IOException as an UncheckedIOException such that
it gets caught by the catch(Throwable) clause in BisqExecutable, which
does print a stacktrace to stderr. This is the way it always should have
worked; throwing ConfigException in these cases was an oversight in the
original implementation. This change also removes the ConfigException
constructor that allows for passing a nested exception to be wrapped as
there is actually no use case for this now that these two anomalies have
been corrected.

This change was made in the process of attempting to reproduce #3998.
@cbeams cbeams force-pushed the cbeams:debug-appdir-creation branch from f5ca539 to de537f0 Feb 24, 2020
@cbeams

This comment has been minimized.

Copy link
Member Author

cbeams commented Feb 25, 2020

Heads up, I am about to push some additional commits to this PR branch that properly fix #3998. I'll update the title and description of the PR accordingly after doing so. Please don't merge in the meantime.

cbeams added 2 commits Feb 25, 2020
This is a pure refactoring that renames and inlines variables to tighten
up and make more consistent the implementation of these two methods. It
is done in preparation for a subsequent substantive change that fixes a
bug.
This change fixes #3998 by avoiding the attempt to create a user's
appDataDir if it already exists. Normally this attempt is not a problem,
as Files#createDirectories does nothing if the target is a directory
that already exists, but in the case that the target is a symbolic link,
Files#createDirectories throws a FileAlreadyExistsException. This change
prevents the call to Files#createDirectories if the target already
exists, and if in fact the target is a symbolic link pointing to an
existing directory, this is not a problem and everything proceeds as per
usual. This mimics the behavior in Bisq v1.2.5 and prior, where
File#mkdirs was used instead of Files#createDirectories. File#mkdirs
does not throw an exception when the target directory is a symlink, it
just returns false.
@cbeams cbeams changed the title Throw UncheckedIOException on directory creation errors Avoid startup failure when appDataDir is a symbolic link Feb 25, 2020
@cbeams

This comment has been minimized.

Copy link
Member Author

cbeams commented Feb 25, 2020

I've updated the title and description of this PR to reflect the actual fix. I've now gotten confirmation from both affected users that they were using symbolic links in their appDataDir paths, so we should be good to go to merge this, i.e. we should feel confident that these changes actually fix the bug.

Copy link
Member

ripcurlx left a comment

utACK

@ripcurlx ripcurlx merged commit db0c76e into bisq-network:master Feb 25, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ripcurlx ripcurlx added this to the v1.2.8 milestone Feb 26, 2020
@ripcurlx ripcurlx mentioned this pull request Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.