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

Provide method to register async shutdown function #613

Conversation

mfb
Copy link
Contributor

@mfb mfb commented Jun 14, 2018

For any custom setups of the client that don't call install(), we should provide a method to register the async shutdown function (_curl_handler is protected so otherwise this can't be done).

@mfb
Copy link
Contributor Author

mfb commented Jun 14, 2018

Looking further info this, we might not need this new method. We could instead modify the existing client registerShutdownFunction() method, allowing it to be run more than once. It needs to run more than once anyways, so I'm not sure why it has this limitation :)

@Jean85
Copy link
Contributor

Jean85 commented Jun 15, 2018

You should not register it more than once, or you would handle the shutdown multiple times.

@mfb
Copy link
Contributor Author

mfb commented Jun 15, 2018

The Raven_CurlHandler::join() method runs 4 times:

  1. It's first registered as a shutdown function by CurlHandler::_construct() calling CurlHandler::registerShutdownFunction().
  2. It's called by Client::onShutdown() which is registered as shutdown function at the end of Client::_construct().
  3. Client::install() calls CurlHandler::registerShutdownFunction() so that CurlHandler::join() runs after fatal error is captured.
  4. Finally, if there wasn't a fatal error, it runs a fourth time in CurlHandler::_destruct().

So I don't think there's anything wrong with registering it more than once :)

@mfb
Copy link
Contributor Author

mfb commented Jun 26, 2018

Since there is no official method to register the shutdown handler that async requires - aside from Client::install() - my current workaround is this:

register_shutdown_function([$client, 'onShutdown']);

@mfb mfb force-pushed the provide-method-to-register-async-shutdown-function branch from 707a3d1 to 515831d Compare October 30, 2018 14:06
register_shutdown_function(array($this, 'onShutdown'));
}
$this->_shutdown_function_has_been_set = true;
register_shutdown_function(array($this, 'onShutdown'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the if removed here? If we can prevent it we should not add all the shutdown handlers...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using curl async, we need to register the shutdown function a second time, so that it runs after the fatal error handler.

$this->_curl_handler->registerShutdownFunction();
}
// Register final shutdown function to send fatal error via curl async.
$this->registerShutdownFunction();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you remove the if here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not necessary, but the idea here was that this would be the suggested way to register shutdown function - given that _curl_handler is protected, this line is useful for anyone who is writing custom code and wants to reproduce the functionality of the install() method.

@stayallive
Copy link
Collaborator

I'm not sure why we should not "protect" it from running more than once. Seems sensible to only register it when needed and only once. No need to register the same handler multiple times.

Fine with making it a public method, also needs some documentation maybe when install() is not used that this is a thing needed with async curl.

@stayallive stayallive changed the title provide method to register async shutdown function Provide method to register async shutdown function Nov 13, 2018
@mfb
Copy link
Contributor Author

mfb commented Nov 13, 2018

I'm not sure why we should not "protect" it from running more than once. Seems sensible to only register it when needed and only once. No need to register the same handler multiple times.

See my comment on June 15 - we are already calling Raven_CurlHandler::join() four times.

This is re: number 3 in my June 15 comment - we need to register the shutdown function again when handling fatal errors with curl async, so that they can be sent after the fatal error is handled.

It should be possible to refactor things so that shutdown functions run as few times as possible - the problem is that it's running too early, making an additional call necessary. Calling registerShutdownFunction() in install() could perhaps replace the other ways that shutdown handlers are being registered?

I wasn't really trying for a major refactoring here, just trying to provide a public method that works, but refactoring would be nice.

@HazAT HazAT changed the base branch from master to 1.x January 14, 2019 09:04
@HazAT
Copy link
Member

HazAT commented Jan 14, 2019

Heads up, I changed the base to 1.x since this is the old master.
Current master branch is the 2.0 version of the SDK.
Version 1.x will receive minor and patch releases from this branch from now on.

@ste93cry
Copy link
Contributor

ste93cry commented Dec 15, 2019

Closing this as 1.x is an old version and I don't think that there is any willing in keeping it updated apart from bug fixes

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.

5 participants