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

Move promotion logic inside workers #38

Merged
merged 1 commit into from
Mar 28, 2023
Merged

Conversation

casperisfine
Copy link
Contributor

@casperisfine casperisfine commented Mar 28, 2023

Fix: #32

This remove a lot of busy work from the server, and allow to promote
as soon as the condition is reached.

The small downside is that we need a lock between processes to ensure
two workers won't try to promote at the same time.

This also means that the mold selector is removed. It wasn't that
interesting anyway, because the Unicorn/Pitchfork architecute introduce
a bias in request distribution, worker#0 will almost always get requests
first so will almost always be the best candidate for promotion.

@casperisfine casperisfine force-pushed the worker-auto-promotion branch 6 times, most recently from 30d0b99 to d5dddcc Compare March 28, 2023 12:23
@casperisfine casperisfine marked this pull request as ready for review March 28, 2023 12:24
@casperisfine casperisfine force-pushed the worker-auto-promotion branch 3 times, most recently from 5483715 to 7dc59c4 Compare March 28, 2023 13:10
Fix: #32

This remove a lot of busy work from the server, and allow to promote
as soon as the condition is reached.

The small downside is that we need a lock between processes to ensure
two workers won't try to promote at the same time.

This also means that the mold selector is removed. It wasn't that
interesting anyway, because the Unicorn/Pitchfork architecute introduce
a bias in request distribution, worker#0 will almost always get requests
first so will almost always be the best candidate for promotion.

class ReforkingTest < Pitchfork::IntegrationTest
if Pitchfork::HttpServer::REFORKING_AVAILABLE
def test_reforking
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 Much easier to read


def initialize(name)
@name = name
@file = Tempfile.create([name, '.lock'])
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own knowledge:

Original problem: Using a POSIX mutex was problematic because if the process holding the mutex died, it wouldn't unlock, and would deadlock the promotion flow thereafter.

Solution: A file lock solves this problem because the lock against the file is automatically released by the OS when the process terminates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I explored several approach to IPC mutexes.

First to use a pthread_mutex_t inside a shared memory page. And as you point out it wouldn't cleanup on exit, which in a context where we might SIGKILL processes ourselves is not OK.

Then I tried sysV semaphores, they do have automatic cleanup, and we already use them via semian, but the API is very wonky to use, especially if you are trying to implement a mutex.

In the end I figured flock was the least worse. It's already available in ruby core, no need for more C, the only downsides are:

  • We need to write a file, which I would have liked to avoid.
  • We need to carefully re-open that file after fork.

But overall it seems to work well.

@casperisfine casperisfine deleted the worker-auto-promotion branch June 23, 2023 10:24
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.

Explore: Move mold promotion logic in workers
3 participants