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

Async (non-blocking) option via executing curl through exec() #132

Merged
merged 2 commits into from
Jun 3, 2014

Conversation

kyle-johnson
Copy link
Contributor

This is one way of addressing #108 and #96.

We use this across many sites logging upwards of 20k events per hour to Sentry. If there is interest in this patch, I will update it for automatic merging.

this enables an async mode; the exec command will run in the background as the script continues execution
@@ -16,7 +16,7 @@

class Raven_Client
{
const VERSION = '0.7.1';
const VERSION = '0.7.1-exec';
Copy link
Member

Choose a reason for hiding this comment

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

can you remove this

@dcramer
Copy link
Member

dcramer commented Feb 6, 2014

I'm a bit concerned as this has to deal w/ PATH and might cause issues on numerous systems. I really think curl_multi is the best solution, but I haven't had time to implement something to try it out.

@dcramer
Copy link
Member

dcramer commented Jun 3, 2014

I'm going to merge this as it's not on by default and there's definitely a need for a solution

@dcramer dcramer merged commit b626fe9 into getsentry:master Jun 3, 2014
@adamlc
Copy link

adamlc commented Jun 19, 2015

@kyle-johnson thanks for writing this. I'm not sure if its easy to fix but this seems to leave loads of zombie processes that aren't being cleaned up properly. I'm using this in a ReactPHP event loop.

root      9209  0.0  0.0      0     0 ?        Z    11:02   0:00 [curl] <defunct>
root      9253  0.0  0.0      0     0 ?        Z    11:02   0:00 [curl] <defunct>
root      9454  0.0  0.0      0     0 ?        Z    11:03   0:00 [curl] <defunct>
root     10208  0.0  0.0      0     0 ?        Z    11:07   0:00 [curl] <defunct>
root     10239  0.0  0.0      0     0 ?        Z    11:07   0:00 [curl] <defunct>
root     10248  0.0  0.0      0     0 ?        Z    11:07   0:00 [curl] <defunct>
root     10319  0.0  0.0      0     0 ?        Z    11:08   0:00 [curl] <defunct>

Not sure if its an easy fix or not? It might be nice for the Raven library to allow for pluggable transports so that we can write our own

@kyle-johnson
Copy link
Contributor Author

@adamlc I wonder if the behavior depends on how PHP is run. I'm using this with php-fpm, and while I have max_requests set to 500, I don't see zombies in the process tree (although I do see a fairly steady stream of errors being logged to Sentry).

Looking at the PHP docs, there doesn't appear to be a way to modify exec's behavior, but I think you could use the pcntl functions (http://php.net/manual/en/book.pcntl.php) to launch the curl process with a bit more control. Perhaps PHP is not ignoring SIGCHILD (that would certainly result in the child processes sticking around as zombies)?

@adamlc
Copy link

adamlc commented Jun 22, 2015

@kyle-johnson thats probably the case. I think it might be better to do a pluggable transport interface so that people can make their own, shouldn't be too difficult. I'll have a stab at it this week!

@popstas
Copy link

popstas commented May 20, 2016

@kyle-johnson, I compared performance of async and exec methods and I see that both methods works well with long response DSN URLs:

$time = microtime(true);

$client = new Raven_Client('http://user:pass@slow.test.dsn.ru/slow.php', [
    'curl_method' => 'exec'
]);

$event_id = $client->getIdent($client->captureMessage('my log message'));

$time = microtime(true) - $time;

echo "time: $time\n";

URL http://slow.test.dsn.ru/slow/index.php just sleeps for 3 seconds and returns 200 with empty body.

With method async script return ~3 ms.
With method exec script return ~5 ms.

What's the advantage of exec over async?

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.

None yet

4 participants