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

Isolate exception state for hooks #118

Merged
merged 5 commits into from
Jan 22, 2024

Conversation

agoallikmaa
Copy link
Contributor

Resolves open-telemetry/opentelemetry-php#1150

This saves the values of the following globals before hook functions are called:

  • exception
  • prev_exception
  • opline_before_exception

These values are then reset to as if there is no exception raised currently. There should ideally be no exception raised at the moment this is called anyway, but this is just in case something has raised an internal exception (could be another observer for example). After the hook function, the old state of those values is restored, and if an exception was thrown by the hook, that is captured separately and logged as a warning.

Tested that the scenario from the issue mentioned above is fixed with this, and added that exact scenario as a test.

Copy link
Collaborator

@brettmc brettmc left a comment

Choose a reason for hiding this comment

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

This looks great! You've probably already noticed some minor complaints about formatting and file references, but otherwise this looks ready to go.

ext/tests/hooks_throw_exception_for_failed_call.phpt Outdated Show resolved Hide resolved
@agoallikmaa
Copy link
Contributor Author

This looks great! You've probably already noticed some minor complaints about formatting and file references, but otherwise this looks ready to go.

Fixed formatting and PECL package list. However there seems to be some genuine bug on Windows with these changes, will investigate that.

@agoallikmaa
Copy link
Contributor Author

Somehow EG(current_execute_data)->opline of the calling frame was not modified in Linux builds if the hook function throws an exception, but it was on Windows builds. Added save&restore for that too, now seems to work correctly on Windows too.

@brettmc brettmc merged commit d4e11b6 into open-telemetry:main Jan 22, 2024
16 checks passed
brettmc added a commit to brettmc/opentelemetry-php-instrumentation that referenced this pull request Jan 22, 2024
if an invalid typehint is provided for first param of post callback, this would previously cause a hang.
fixed by previous open-telemetry#118, this test confirms the fix works in this scenario.
brettmc added a commit to brettmc/opentelemetry-php-instrumentation that referenced this pull request Jan 22, 2024
if an invalid typehint is provided for first param of post callback, this would previously cause a hang.
fixed by open-telemetry#118, this test confirms the fix works in this scenario.
brettmc added a commit that referenced this pull request Jan 23, 2024
* adding test for post hook type error
if an invalid typehint is provided for first param of post callback, this would previously cause a hang.
fixed by #118, this test confirms the fix works in this scenario.

* improve test, add to package.xml

* remove try/catch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants