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: improve response time on windows when using deadline config GUI #540

Merged
merged 2 commits into from
Dec 20, 2024

Conversation

AWS-Samuel
Copy link
Contributor

@AWS-Samuel AWS-Samuel commented Dec 19, 2024

What was the problem/requirement? (What/Why)

On Windows, there was a 10+ second hang whenever updating settings through the deadline config gui GUI when apply or OK were clicked.

This hang was caused by opening a subprocess to call icacls in order to set and read directory permissions.

This lead to discovering that we were erroneously resetting the permissions on that directory on each config change.

What was the solution? (How)

We only need to set directory permissions when creating the directory, so add logic to detect if we are creating the directory.

Also switch to using the win32security module to set permissions instead of opening a subprocess that calls icacls. This is faster and doesn't rely on interpreting the STDOUT of another process.

What is the impact of this change?

The GUI is more responsive when applying or saving settings.

How was this change tested?

See DEVELOPMENT.md for information on running tests.

  • Have you run the unit tests?
    Yes
  • Have you run the integration tests?
    Yes
  • Have you made changes to the download or asset_sync modules? If so, then it is highly recommended
    that you ensure that the docker-based unit tests pass.
    No

I also tested manually by comparing the response time when using the config GUI before and after this change.

  • Before the change there was a 10ish second hang when clicking Apply before the GUI could be interacted with again.
  • After the change the GUI was immediately responsive (at least, as fast as I could move my mouse from apply to another GUI element)

For the permissions change, I added unit tests, and also manually tested by:

  • Deleting my ~/.deadline folder, and running deadline config gui to initialize the directory + the config file.
    • Confirmed that the directory had permissions for the SYSTEM account, domain Administrators, and my logged in username
  • Modified permissions on the directory, and re-ran deadline config gui.
    • Confirmed that the directory permissions had not changed, and that my modified permissions remained intact.

Was this change documented?

  • Are relevant docstrings in the code base updated?
    • No relevant docstrings
  • Has the README.md been updated? If you modified CLI arguments, for instance.
    • No README.md changed to make

Does this PR introduce new dependencies?

This library is designed to be integrated into third-party applications that have bespoke and customized deployment environments. Adding dependencies will increase the chance of library version conflicts and incompatabilities. Please evaluate the addition of new dependencies. See the Dependencies section of DEVELOPMENT.md for more details.

  • This PR adds one or more new dependency Python packages. I acknowledge I have reviewed the considerations for adding dependencies in DEVELOPMENT.md.
  • This PR does not add any new dependencies.

Is this a breaking change?

This change is not a breaking change

Does this change impact security?

Yes-ish.

It modifies a part of the code where we set file permissions, and removes the part where we always overwrite existing file permissions.

The actual permissions which are set have not been changed. I manually validated this by creating the directory through deadline config gui, and confirming that the permission on the folder where identical upon first creation both before and with this change.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@AWS-Samuel AWS-Samuel requested a review from a team as a code owner December 19, 2024 22:34
@AWS-Samuel AWS-Samuel force-pushed the fix-gui-response-time branch from a032b92 to 00cd134 Compare December 19, 2024 22:35
@epmog epmog added security Pull requests that could impact security bug Something isn't working labels Dec 19, 2024
mwiebe
mwiebe previously approved these changes Dec 19, 2024
test/unit/deadline_client/config/test_config_file.py Outdated Show resolved Hide resolved
@AWS-Samuel AWS-Samuel force-pushed the fix-gui-response-time branch from c69cd45 to 6268a3c Compare December 20, 2024 17:10
@AWS-Samuel AWS-Samuel enabled auto-merge (squash) December 20, 2024 18:12
@AWS-Samuel AWS-Samuel merged commit 6873156 into aws-deadline:mainline Dec 20, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security Pull requests that could impact security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants