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.

Sign up for GitHub

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

Jump to bottom

Only ping worker 0 during phased restart if using fork worker #3568

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Only ping worker 0 during phased restart if using fork worker #3568

merged 1 commit into from
Dec 11, 2024

Conversation

joshuay03
Copy link
Contributor

Description

The ping added in #3225 is only needed when forking workers from worker 0 during a phased restart (i.e., fork worker is enabled); however, it is currently called during an ordinary phased restart as well.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@github-actions github-actions bot added the waiting-for-review Waiting on review from anyone label Dec 1, 2024
@nateberkopec nateberkopec merged commit 94461f4 into puma:master Dec 11, 2024
78 checks passed
@joshuay03 joshuay03 deleted the only-ping-worker-0-on-phased-restart-if-fork-worker branch December 11, 2024 23:22
@joshuay03
Copy link
Contributor Author

Gah I forgot a refork can be triggered with SIGURG, so this is not a good solution for that case. I'll open a follow-up PR soon where we don't rely on the config option for this condition.

@joshuay03
Copy link
Contributor Author

joshuay03 commented Dec 14, 2024 •
edited
Loading

Gah I forgot a refork can be triggered with SIGURG, so this is not a good solution for that case. I'll open a follow-up PR soon where we don't rely on the config option for this condition.

Ignore me, I assumed that a refork can be triggered at any time with SIGURG, even when fork_worker has not been configured (at least that's the impression I got from the docs), but that's not the case.

So this patch is fine as is i.e. it covers both refork on hitting configured request count and refork on receiving SIGURG. I've got a PR up to add a test that covers the the signal case: #3579

@joshuay03 joshuay03 mentioned this pull request Dec 14, 2024
Open
7 tasks
@dentarg dentarg added perf and removed waiting-for-review Waiting on review from anyone labels Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
perf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshuay03 @dentarg @nateberkopec