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.

Sign up for GitHub

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

Fix OpenStudio Windows crashed during save attempt #4026

Merged
merged 7 commits into from
Aug 7, 2020
Merged

Fix OpenStudio Windows crashed during save attempt #4026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7de76c9
Update tag to rc2
tijcolem Jun 19, 2020
a30f69b
Merge branch 'develop' of https://github.com/NREL/OpenStudio into 3.0…
tijcolem Jun 22, 2020
09b7c8a
tag and release 3.0.1
tijcolem Jun 25, 2020
b43d1d2
Update to not crash on save if directory is open in Windows explorer
macumber Jul 26, 2020
c0d8ae9
Add release and debug to github
macumber Jul 26, 2020
73db0ed
Didn't mean to change this file
macumber Jul 31, 2020
59d6a2a
empty commit
macumber Aug 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ test_gems
test_fails.txt
# Ignore build folders (at root level only)
/build*/
/debug*/
/release*/

developer/msvc/Visualizers/all_concat.natvis
.vscode/
Expand Down
41 changes: 24 additions & 17 deletions src/model/FileOperations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ namespace openstudio {
namespace model {
bool replaceDir(const openstudio::path &sourceDir, const openstudio::path &destinationDir)
{
bool result = true;

// Path **must** exist for `canonical()` to work, `weakly_canonical()` doesn't require this
const auto src = openstudio::filesystem::weakly_canonical(sourceDir);
const auto dest = openstudio::filesystem::weakly_canonical(destinationDir);
Expand All @@ -100,21 +102,22 @@ namespace model {
}

// If the source dir doesn't exist, there's a problem
if (!openstudio::filesystem::exists(src) || !openstudio::filesystem::is_directory(src))
{
if (!openstudio::filesystem::exists(src) || !openstudio::filesystem::is_directory(src)) {
LOG_FREE(Error, "replaceDir", "Source directory does not exist: " << toString(src));
return false;
}

// If the dest dir exists, remove it
if (openstudio::filesystem::exists(dest))
{
// If the dest dir exists, remove all entries in it
if (openstudio::filesystem::exists(dest)) {
if (openstudio::filesystem::is_directory(dest)) {
try {
openstudio::filesystem::remove_all(dest);
} catch (const std::exception &e) {
LOG_FREE(Error, "replaceDir", "Destination directory could not be removed: " << toString(dest) << "error: " << e.what());
return false;
for (openstudio::filesystem::directory_iterator end_dir_it, it(dest); it != end_dir_it; ++it) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So before: try to remove entire companion dir.
Now : try to remove all files directly under companion dir, and all subfolders.

To avoid more problems (eg a single file is opened with a handle on it in the files/ subfolder), we should do the same logic recursively, no?

Make safeDeleteDir() function or something: if file try delete, if directory call safeDeleteDir.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that would work too, but the main thing is don't throw an unhandled exception if we try to delete something (file or folder) and it fails. Just log the file and return false. We should try to clean out as many files as we can.

openstudio::filesystem::path path = it->path();
try {
openstudio::filesystem::remove_all(path);
} catch (const std::exception& e) {
LOG_FREE(Error, "replaceDir", "Error deleting: " << toString(path) << e.what());
result = false;
}
}
} else {
LOG_FREE(Error, "replaceDir", "Destination exists and is not a directory, aborting: " << toString(dest));
Expand All @@ -123,15 +126,19 @@ namespace model {
}

// Now we recreate it, and it must work!
if (!openstudio::filesystem::create_directory(dest))
{
LOG_FREE(Error, "replaceDir", "Could not create destination directory: " << toString(dest));
return false;
if (!openstudio::filesystem::exists(dest)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@macumber As far as I can tell you're actually no longer trying to remove the directory itself above (should you be after you tried removing everything inside?), so this block is pointless no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I think you can hit this if you open a bare OSM file with no companion directory, the companion directory would be created in temp and then would be synched back.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

try {
if (!openstudio::filesystem::create_directory(dest)) {
LOG_FREE(Error, "replaceDir", "Could not create destination directory: " << toString(dest));
return false;
}
} catch (const std::exception& e) {
LOG_FREE(Error, "replaceDir", "Error creating directory: " << toString(dest) << e.what());
return false;
}
}

bool result = true;
for (const auto& dirEnt : openstudio::filesystem::recursive_directory_iterator{src})
{
for (const auto& dirEnt : openstudio::filesystem::recursive_directory_iterator{src}) {
const auto& path = dirEnt.path();
auto relativePathStr = path.string();
boost::replace_first(relativePathStr, src.string(), "");
Expand Down