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

Add simple demo framework for Bedrock::Jobs; add requestID; add retries #15

Merged
merged 19 commits into from Apr 17, 2017
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 35 additions & 18 deletions bin/BedrockWorkerManager.php
Expand Up @@ -71,34 +71,37 @@
Client::configure($bedrockConfig);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iwiznia - Why do you manually copy all the values over to $bedrockConfig before passing it in, rather than just passing in $options?


$logger = Client::getLogger();
$logger->info('Starting BedrockWorkerManager', ['maxLoopIteration' => $maxLoopIteration]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moving this line further up to keep the logging initialization code as early as possible.


$stats = Client::getStats();
$versionWatchFileTimestamp = $versionWatchFile && file_exists($versionWatchFile) ? filemtime($versionWatchFile) : false;

// Wrap everything in a general exception handler so we can handle error
// conditions as gracefully as possible.
try {
$logger->info('Starting BedrockWorkerManager', ['maxLoopIteration' => $maxLoopIteration]);

if (!file_exists('/proc/loadavg')) {
throw new Exception('are you in a chroot? If so, please make sure /proc is mounted correctly');
}

// Connect to Bedrock -- it'll reconnect if necessary
$bedrock = new Client();
$jobs = new Jobs($bedrock);

// Begin the infinite loop
// Loop a finite number of times and then self destruct. This is to guard
// against memory leaks, as we assume there is some other script that will
// restart this when it dies.
$loopIteration = 0;
while (true) {
// Is it time to self destruct?
if ($loopIteration === $maxLoopIteration) {
$logger->info("We did all our loops iteration, shutting down");
exit(0);
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exit(0) skips the "wait for children to finish" code at the end.

}

$loopIteration++;
$logger->info("Loop iteration", ['iteration' => $loopIteration]);

// Step One wait for resources to free up
while (true) {
// Get the latest load
if (!file_exists('/proc/loadavg')) {
throw new Exception('are you in a chroot? If so, please make sure /proc is mounted correctly');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the /proc/loadavg check here to keep it next to the sys_getloadavg() call that depends on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to do this check every time the loop runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed we don't need to, but I think it's non-harmful to do it (eg, it's not a super expensive call given the relatively low the frequency of this loop), and makes the code read a lot cleaner to keep it where it's used.

$load = sys_getloadavg()[0];
if ($load < $maxLoad) {
$logger->info('Load is under max, checking for more work.', ['load' => $load, 'MAX_LOAD' => $maxLoad]);
Expand All @@ -109,21 +112,28 @@
}
}

// Get any job managed by the BedrockWorkerManager
// Poll the server until we successfully get a job
$response = null;
while (!$response) {
// php's filemtime results are cached, so we need to clear that cache or we'll be getting a stale modified time.
// Watch a version file that will cause us to automatically shut
// down if it changes. This enables triggering a restart if new
// PHP is deployed.
//
// Note: php's filemtime results are cached, so we need to clear
// that cache or we'll be getting a stale modified time.
clearstatcache(true, $versionWatchFile);
$newVersionWatchFileTimestamp = $versionWatchFile && file_exists($versionWatchFile) ? filemtime($versionWatchFile) : false;
$newVersionWatchFileTimestamp = ($versionWatchFile && file_exists($versionWatchFile)) ? filemtime($versionWatchFile) : false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just clarifying what this means with parenthesis as the precedence of the operators wasn't obvious to me at first.

if ($versionWatchFile && $newVersionWatchFileTimestamp !== $versionWatchFileTimestamp) {
$logger->info('Version watch file changed, stop processing new jobs');

// We break out of this loop and the outer one too. We don't want to process anything more,
// just wait for child processes to finish.
break 2;
}

// Query the server for a job
try {
// Attempt to get a job
// Call GetJob
$response = $jobs->getJob($jobName, 60 * 1000); // Wait up to 60s
} catch (Exception $e) {
// Try again in 60 seconds
Expand Down Expand Up @@ -166,11 +176,15 @@
$logger->info("Looking for worker '$workerFilename'");
if (file_exists($workerFilename)) {
// The file seems to exist -- fork it so we can run it.
//
// Note: By explicitly ignoring SIGCHLD we tell the kernel to
// "reap" finished child processes automatically, rather
// than creating "zombie" processes. (We don't care
// about the child's exit code, so we have no use for the
// zombie process.)
$logger->info("Forking and running a worker.", [
'workerFileName' => $workerFilename,
]);
// Ignore SIGCHLD signal, which should help 'reap' zombie processes, forcing zombies to kill themselves
// in the event that the parent process dies before the child/zombie)
pcntl_signal(SIGCHLD, SIG_IGN);
$pid = pcntl_fork();
if ($pid == -1) {
Expand Down Expand Up @@ -245,10 +259,13 @@
}
}

// We wait for all children to finish before dying.
$status = null;
pcntl_wait($status);
} catch (Exception $e) {
$message = $e->getMessage();
$logger->alert('BedrockWorkerManager.php exited abnormally', ['exception' => $e]);
}

// We wait for all children to finish before dying.
$logger->info('Stopping BedrockWorkerManager, waiting for children');
$status = null;
pcntl_wait($status);
$logger->info('Stopped BedrockWorkerManager');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving all this logic to the end of the outer loop, so it runs regardless of whether we're doing a graceful or abnormal shutdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iwiznia Are you sure this shutdown code does anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.