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: atexit hook to clean up cache threads, TyphosSuite garbage collection #516

Merged
merged 11 commits into from
Sep 6, 2022

Conversation

klauer
Copy link
Contributor

@klauer klauer commented Sep 1, 2022

Description

  • Add cleanup for background threads
  • Add replacement for functools.partial usage in methods as this was causing TyphosSuite from getting garbage collected

Motivation and Context

  • Dangling cache daemon threads appear to cause a segfault on pip-installed PyQt5 but not conda-forge PyQt5
  • I tried the signal QApplication.aboutToQuit instead of atexit, thinking it'd be better to clean up when the QApplication does. I found that the slot wasn't getting called and fell back to atexit.
  • The garbage collection issue led to the test suite failing in a very strange way:
    • The benchmarks in test_benchmark are configured to run and create very nested device displays. These device displays were supposed to get garbage collected between tests, but they were not.
    • Once the test suite reached test_cli, typhos instructed Qt to reload stylesheets for every widget in the application.
    • This caused massive amounts of recursive updates of style sheets on a ton (?) of widgets from every previous test. This bogged the test suite down so much that it used to take over 20 minutes to run. This proposal should hopefully fix that.

How Has This Been Tested?

Where Has This Been Documented?

PRs and issues

@klauer klauer closed this Sep 1, 2022
@klauer klauer reopened this Sep 1, 2022
@tangkong
Copy link
Contributor

tangkong commented Sep 1, 2022

This works for me inside a docker container that reproduces travis testing conditions, removing the QThread teardown failures at the end of the pytest suite. Other test suite bugs are still in travis however

@klauer klauer changed the title FIX: atexit hook to clean up cache threads FIX: atexit hook to clean up cache threads, TyphosSuite garbage collection Sep 2, 2022
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I think I need to do my homework and finally read up on how all the weakref mechanisms work

All of this looks good to me

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

This looks good to me. The test suite passes on Travis, so I'd say the proof is in the pudding.

This left me wondering what other apps might benefit from garbage collection utilities like this. I suppose we won't really know until we hit upon more problems, but if it's been solved once it can be solved again!

typhos/suite.py Outdated
QtCore.Qt.QueuedConnection)
parameter.sigHide.connect(partial(self.hide_subdisplay, parameter),
QtCore.Qt.QueuedConnection)
self._connect_partial(
Copy link
Contributor

Choose a reason for hiding this comment

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

To make sure I have this straight, TyphosBase._connect_partial connects a WeakPartialMethodSlot to a signal.

  • Should we name _connect_partial -> _weak_connect_partial or something that makes it obvious that weak refs are being used? I can see myself forgetting that and thinking _connect_partial is just a wrapper around functools.partial

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tweaked the name - going to merge and re-run CI on another long-open PR

@klauer klauer merged commit 76486e3 into pcdshub:master Sep 6, 2022
@klauer klauer deleted the fix_cache_threads branch September 6, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants