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

Reset directory on failed file interpretation #1770

Merged
merged 7 commits into from
Dec 22, 2022

Conversation

samcowger
Copy link
Contributor

When includeing a script in a different directory, failure to load the file will leave the interpreter's working directory as the directory containing that script. This fixes that by catching any exceptions that are thrown during file load, resetting the dirctory, and rethrowing.

Once #1769 is merged, this behavior should be amenable to automated testing. As it is, the directory is reset on interpreter exit, so neither the failure mode nor its resolution can be encoded in a test script.

src/SAWScript/Value.hs Outdated Show resolved Hide resolved
src/SAWScript/Value.hs Outdated Show resolved Hide resolved
src/SAWScript/Interpreter.hs Outdated Show resolved Hide resolved
@samcowger samcowger marked this pull request as ready for review December 21, 2022 21:14
@kquick
Copy link
Member

kquick commented Dec 21, 2022

The implementation here looks good, I would just make a general observation that in libraries it's not usually advisable to make process-global changes like changing the current working directory because it can have deleterious effects on other threads or expectations of the application using the library. At present this interpreter block probably represents the main thread of saw itself so it's less of an immediate concern, but in general it's probably better to remember target directories (e.g. in a ReaderT context) and then apply them explicitly when doing any file operations.

@samcowger
Copy link
Contributor Author

This should, for the record, finally fix a bug mentioned in #1341.

@samcowger
Copy link
Contributor Author

The implementation here looks good, I would just make a general observation that in libraries it's not usually advisable to make process-global changes like changing the current working directory because it can have deleterious effects on other threads or expectations of the application using the library. At present this interpreter block probably represents the main thread of saw itself so it's less of an immediate concern, but in general it's probably better to remember target directories (e.g. in a ReaderT context) and then apply them explicitly when doing any file operations.

Noted and agreed. @kquick and I spoke and agreed that this PR is still appropriate to merge, but that recording this issue would be wise, so I've opened #1791 to surface it more broadly.

@samcowger samcowger added the PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run label Dec 22, 2022
@mergify mergify bot merged commit b4ecd82 into master Dec 22, 2022
@mergify mergify bot deleted the bugfix/interpreter-directory-reset branch December 22, 2022 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants