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

Refactor fx service Start/Stop #4584

Merged
merged 5 commits into from
Jul 6, 2023
Merged

Refactor fx service Start/Stop #4584

merged 5 commits into from
Jul 6, 2023

Conversation

dnr
Copy link
Member

@dnr dnr commented Jul 6, 2023

What changed?

  • Change the four main Service Start methods to no longer block
  • But also run synchronously in fx lifecycle (except for the final Serve call)
  • Use fx.StartStopHook to simplify code
  • Get rid of s.status atomic ops since fx will serialize Start/Stop
  • Get rid of stopChan as well

Why?
Allow more of service start/stop to be serialized by fx lifecycle, to avoid bugs when e.g. Stop is called while Start is still running.

How did you test it?
manually and integration tests

Potential risks

Is hotfix candidate?

Copy link
Contributor

@MichaelSnowden MichaelSnowden left a comment

Choose a reason for hiding this comment

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

🏡

@dnr
Copy link
Member Author

dnr commented Jul 6, 2023

Just FYI, there's still a bug (#4174) you can observe with the dev server where if you let it finish starting, it'll stop within a couple seconds (on interrupt signal). But if you interrupt it before it finishes starting, it'll take 10 seconds. I'm not sure exactly where that is yet, but it was there before this PR also. In theory this might make that easier to find/fix

@MichaelSnowden
Copy link
Contributor

Just FYI, there's still a bug (#4174) you can observe with the dev server where if you let it finish starting, it'll stop within a couple seconds (on interrupt signal). But if you interrupt it before it finishes starting, it'll take 10 seconds. I'm not sure exactly where that is yet, but it was there before this PR also. In theory this might make that easier to find/fix

I have a hunch that it's #3818 or something of a similar nature (wg.Wait() in a stop method). I think you plan on landing this PR regardless of that, which I totally support. If we do want to troubleshoot that bug, I think fx logs something before each stop method, right? And, if not, probably there's a hook in the fx logger we can utilize.

@dnr dnr mentioned this pull request Jul 6, 2023
@dnr dnr marked this pull request as ready for review July 6, 2023 20:23
@dnr dnr requested a review from a team as a code owner July 6, 2023 20:23
@dnr dnr merged commit 92c8418 into temporalio:master Jul 6, 2023
@dnr dnr deleted the fx8 branch July 6, 2023 22:17
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