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

Fix OpenStudio Windows crashed during save attempt #4026

Merged
merged 7 commits into from
Aug 7, 2020

Conversation

macumber
Copy link
Contributor

Pull request overview

  • Currently replaceDir will throw an uncaught exception if the user has the save directory (or child subdirectory) open in Windows explorer. This change no longer deletes the save directory if that exists, it clears all contents inside of it. Code attempts to sync as many changes as possible and does not generate uncaught exceptions.

Pull Request Author

  • [ x ] Checked behavior in OpenStudioApplication, adjusted policies as needed (src/openstudio_lib/library/OpenStudioPolicy.xml)

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • Code Style, strip trailing whitespace, etc.
  • All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
  • Labeling is ok
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified

macumber added a commit to openstudiocoalition/OpenStudioApplication that referenced this pull request Jul 26, 2020
macumber added a commit to openstudiocoalition/OpenStudioApplication that referenced this pull request Jul 26, 2020
@tijcolem tijcolem changed the title Master Fix OpenStudio Windows crashed during save attempt Jul 29, 2020
@tijcolem tijcolem added the Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. label Jul 31, 2020
@tijcolem tijcolem added this to the OpenStudio SDK 3.1.0 milestone Jul 31, 2020
@jmarrec
Copy link
Collaborator

jmarrec commented Aug 3, 2020

Windows is offline, and mac is failing on the usual suspect 2366 - EnergyPlusFixture.ForwardTranslatorTest_MultiThreadedLogMessages (Failed)

@jmarrec jmarrec self-requested a review August 3, 2020 10:25
{
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!

} 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.

@tijcolem
Copy link
Collaborator

tijcolem commented Aug 7, 2020

@macumber Thanks for the fix for this! I know many app users will be very happy as this has been frustrating when saving your work only to have it crash. This looks good to drop to me.

@tijcolem tijcolem merged commit cba3c6a into NREL:develop Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component - Utilities Other Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. severity - Major Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants