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

Bugfix: start JobStatusPoller at engine start #1803

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

khk-globus
Copy link
Contributor

@khk-globus khk-globus commented Feb 13, 2025

The recent refactor to move the JobStatusPoller thread initialization to the GlobusComputeEngine.__init__ method missed that endpoints are often daemonized. Daemonization only allows the fork()ing thread to survive.

Fix: Move the JSP thread creation back to the GCE's .start() method, since this method is invoked after the daemonization. This time, cement the expectation with a test.

Reference:

Type of change

  • Bug fix (non-breaking change that fixes an issue)

The recent refactor to move the JobStatusPoller thread initialization to the
`GlobusComputeEngine.__init__` method missed that endpoints are often
daemonized.  Daemonization only allows the fork()ing thread to survive.

Fix: Move the JSP thread creation back to the GCE's `.start()` method, since
this method is invoked *after* the daemonization.  This time, cement the
expectation with a test.

Reference:

    - #1779 (as part of [sc-37953])
    - 393b410
Copy link
Contributor

@LeiGlobus LeiGlobus left a comment

Choose a reason for hiding this comment

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

It's late night work, but nicely done!

We should also spin up an endpoint with at least the MVP 'happy' path after deploys. I only test the newly deployed Tutorial MEP sometimes, lesson learned.

@LeiGlobus LeiGlobus merged commit 7c7eb6e into 3.1.1 Feb 13, 2025
11 checks passed
@LeiGlobus LeiGlobus deleted the bug/delay_start_jsp_thread branch February 13, 2025 05:38
@khk-globus
Copy link
Contributor Author

We should also spin up an endpoint with at least the MVP 'happy' path after deploys. I only test the newly deployed Tutorial MEP sometimes, lesson learned.

Hah. That's exactly what I thought I did! But clearly missed the detach_endpoint distinction. Alas. Hopefully the UT helps with that particular detail going forward.

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.

2 participants