fix(agent): Fix error reporting when EH_THROW is enabled #876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
/* * For a the following error codes: * E_WARNING || E_CORE_WARNING || E_COMPILE_WARNING || E_USER_WARNING * PHP triggers an exception if EH_THROW is toggled on and then immediately * returns after throwing the exception. See for more info: * https://github.com/php/php-src/blob/master/main/main.c In that case, we * should not handle it, but we should exist and let the exception handler * deal with it; otherwise, we could record an error even if an exception is * caught. */
|
/* * For a the following error codes: * E_WARNING || E_CORE_WARNING || E_COMPILE_WARNING || E_USER_WARNING * PHP triggers an exception if EH_THROW is toggled on and then immediately * returns after throwing the exception. See for more info: * https://github.com/php/php-src/blob/master/main/main.c In that case, we * should not handle it, but we should exist and let the exception handler * deal with it; otherwise, we could record an error even if an exception is * caught. */
zsistla
force-pushed
the
handle_EH_THROW
branch
from
7d8b174
to
0db7169
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #876 +/- ##
=======================================
Coverage 78.76% 78.76%
=======================================
Files 193 193
Lines 27125 27127 +2
=======================================
+ Hits 21365 21367 +2
Misses 5760 5760
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ZNeumann
reviewed
Apr 17, 2024
ZNeumann
approved these changes
Apr 17, 2024
Co-authored-by: ZNeumann <zneumann@newrelic.com>
lavarou
reviewed
Apr 18, 2024
lavarou
reviewed
Apr 18, 2024
lavarou
reviewed
Apr 18, 2024
tests/integration/errors/test_EH_THROW_errors_caught_exception.php
Outdated
Show resolved
Hide resolved
lavarou
reviewed
Apr 18, 2024
tests/integration/errors/test_EH_THROW_errors_uncaught_exception.php
Outdated
Show resolved
Hide resolved
lavarou
reviewed
Apr 18, 2024
There was a problem hiding this comment.
I'd like to know the answer to this question before I approve.
Co-authored-by: Michal Nowacki <mnowacki@newrelic.com>
….php Co-authored-by: Michal Nowacki <mnowacki@newrelic.com>
…on.php Co-authored-by: Michal Nowacki <mnowacki@newrelic.com>
lavarou
reviewed
Apr 18, 2024
tests/integration/errors/test_EH_THROW_errors_caught_exception.php
Outdated
Show resolved
Hide resolved
lavarou
reviewed
Apr 18, 2024
Co-authored-by: Michal Nowacki <mnowacki@newrelic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR
1)checks if EH_THROW(which signals to PHP to throw an exception from the error) is toggled and if so does not record an error because if uncaught it will be handled by the exception handler and if caught there's no need to record it.
2) add tests to verify fix
More details:
/*
In the case where a php E_WARNING error was triggered. We intercept the error from PHP and record it then pass execution back to PHP.
But, for instance, in the particular case that deals with a filehandling which toggles EH_THROW on here: https://github.com/php/php-src/blob/master/ext/fileinfo/fileinfo.c#L176
PHP then handles the error but for a certain number of error codes, PHP triggers an exception if EH_THROW is toggled on: https://github.com/php/php-src/blob/master/main/main.c#L1254C1-L1259C24
and then immediately returns after throwing the exception.
This leads to the case in the issue where the exception that was triggered is caught, but we recorded the error....