fix(agent): improve exception handler instrumentation for PHPs 8.0+ #877

lavarou · 2024-04-12T16:29:56Z

When user exception handler is installed it can call restore_exception_handler,
and that will reset is_exception_handler flag in the wraprec which will cause
the agent not to use the correct code path after user exception handler returns.
The information that nr_php_instrument_func_end is handling return from user
exception handler needs to be stored in the segment associated with the user
exception handler.

This test reproduces the issue of the agent not recording the error when exception handler calls restore_exception_handler.

When user exception handler is installed it can call restore_exception_handler, and that will reset is_exception_handler flag in the wraprec which will cause the agent not to use the correct code path after user exception handler returns. The information that `nr_php_instrument_func_end` is handling return from user exception handler needs to be stored in the segment associated with the user exception handler.

newrelic-php-agent-bot · 2024-04-12T16:30:18Z

Test Suite	Status	Result
Multiverse	✅	9/9 passing
SOAK	✅	51/51 passing

codecov-commenter · 2024-04-12T16:54:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.76%. Comparing base (e0d2d12) to head (5013256).
Report is 2 commits behind head on dev.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #877   +/-   ##
=======================================
  Coverage   78.76%   78.76%           
=======================================
  Files         193      193           
  Lines       27125    27129    +4     
=======================================
+ Hits        21365    21369    +4     
  Misses       5760     5760

Flag	Coverage Δ
agent-for-php-7.0	`77.82% <ø> (+<0.01%)`	⬆️
agent-for-php-7.1	`77.58% <ø> (+<0.01%)`	⬆️
agent-for-php-7.2	`78.15% <ø> (+<0.01%)`	⬆️
agent-for-php-7.3	`78.17% <ø> (+<0.01%)`	⬆️
agent-for-php-7.4	`77.87% <ø> (+<0.01%)`	⬆️
agent-for-php-8.0	`77.90% <100.00%> (+<0.01%)`	⬆️
agent-for-php-8.1	`77.88% <100.00%> (+<0.01%)`	⬆️
agent-for-php-8.2	`77.48% <100.00%> (+<0.01%)`	⬆️
agent-for-php-8.3	`77.48% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Agent for PHPs 8.0+ uses observer API to hook into Zend Engine, which allows offers better out-of-the-box instrumentation of user exception handler. When observer API is used, there's no need to wrap exception handlers to record errors.

agent/php_execute.c

There's no need to check if wraprec is not NULL - it is guaranteed to be not NULL at this point.

tests/integration/errors/test_oapi_uncaught_handled_exception_04.php

Co-authored-by: ZNeumann <[email protected]>

zsistla · 2024-04-18T16:03:49Z

Do you have an idea what is causing the 8.3 failure?

FAIL - tests/integration/errors/test_oapi_uncaught_handled_exception_04.php
regex error:
expected:
Fatal error: Uncaught RuntimeException: Not able to handle, throwing another exception from handler

actual:
01 Handled uncaught exception

lavarou · 2024-04-18T16:45:40Z

Do you have an idea what is causing the 8.3 failure?

FAIL - tests/integration/errors/test_oapi_uncaught_handled_exception_04.php
regex error:
expected:
Fatal error: Uncaught RuntimeException: Not able to handle, throwing another exception from handler

actual:
01 Handled uncaught exception

@zsistla Good eye! This means that PHP 8.3 started to behave like I would expect all PHPs to behave, that is honor the exception handler's stack. That was the purpose of this test! I remember reading about it somewhere... I need to split this test into two tests: for PHPs < 8.3 and for PHPs >= 8.3.

lavarou · 2024-04-18T17:33:11Z

Do you have an idea what is causing the 8.3 failure?
FAIL - tests/integration/errors/test_oapi_uncaught_handled_exception_04.php
regex error:
expected:
Fatal error: Uncaught RuntimeException: Not able to handle, throwing another exception from handler

actual:
01 Handled uncaught exception
@zsistla Good eye! This means that PHP 8.3 started to behave like I would expect all PHPs to behave, that is honor the exception handler's stack. That was the purpose of this test! I remember reading about it somewhere... I need to split this test into two tests: for PHPs < 8.3 and for PHPs >= 8.3.

@zsistla This is really broken in PHP. Even not all versions of PHP 8.3 behave the same: https://3v4l.org/ARJ9A

PHP 8.3 introduced changes to handling exceptions and honoring exception handler stack. Add tests that illustrate those behavior differences.

lavarou · 2024-04-18T19:59:33Z

Do you have an idea what is causing the 8.3 failure?
FAIL - tests/integration/errors/test_oapi_uncaught_handled_exception_04.php
regex error:
expected:
Fatal error: Uncaught RuntimeException: Not able to handle, throwing another exception from handler

actual:
01 Handled uncaught exception
@zsistla Good eye! This means that PHP 8.3 started to behave like I would expect all PHPs to behave, that is honor the exception handler's stack. That was the purpose of this test! I remember reading about it somewhere... I need to split this test into two tests: for PHPs < 8.3 and for PHPs >= 8.3.
@zsistla This is really broken in PHP. Even not all versions of PHP 8.3 behave the same: https://3v4l.org/ARJ9A

Tried to address this with 4646c9c.

zsistla · 2024-04-18T20:33:53Z

tests/integration/errors/test_oapi_uncaught_handled_exception_04.php835.php

+
+/*SKIPIF
+<?php
+if (version_compare(PHP_VERSION, "8.3", "<")) {


test is named php835, but this just checks for 8.3?

I'm not sure how to properly name the test files that need to run for PHPs 8.3.0-8.3.4 (currently the test is named tests/integration/errors/test_oapi_uncaught_handled_exception_04.php83.php) vs test files that run correctly for all 8.3.x PHPs. I'm open to suggestions.

The 835 naming convention was perfectly fine, but the 835 test only had this check:

<?php if (version_compare(PHP_VERSION, "8.3", "<")) { die("skip: PHP < 8.3.0 not supported\n"); }

shouldn't it be

if (version_compare(PHP_VERSION, "8.3.5", "<")) { die("skip: PHP < 8.3.5 not supported\n"); }

I assumed 835 meant php 8.3.5 and above.

Maybe instead of in the name, update the description inside the test explaining why the skipif and which versions we expect it to run on.
Then you can have an 83a, 83b, etc to indicate it is the same test but differences are needed due to variants 8.3?

Thanks for the suggestions. See 5013256.

zsistla · 2024-04-18T20:38:00Z

Not a blocker, but why do the text cases need _oapi_ in the name? It doesn't seem to meaningfully add information about the test case.

lavarou · 2024-04-18T20:42:09Z

Not a blocker, but why do the text cases need _oapi_ in the name? It doesn't seem to meaningfully add information about the test case.

I included _oapi_ in the name because they only exercise _oapi_ codepaths. I'm open to suggestions how to better convey that information without including _oapi_ in the name.

Explain why different tests are needed in test description rather than test filename.

lavarou · 2024-04-18T21:56:09Z

Not a blocker, but why do the text cases need _oapi_ in the name? It doesn't seem to meaningfully add information about the test case.

I included _oapi_ in the name because they only exercise _oapi_ codepaths. I'm open to suggestions how to better convey that information without including _oapi_ in the name.

For 8+ there are only oapi code paths, we didn't include oapi on any other 8+ only tests or non-oapi on any pre8 tests. just seemed redundant.

Removed _oapi part from test filename in e8669dd.

zsistla · 2024-04-18T22:54:15Z

LGTM!

lavarou added 2 commits April 12, 2024 11:40

add a test that reproduces the issue

b6191bd

This test reproduces the issue of the agent not recording the error when exception handler calls restore_exception_handler.

lavarou requested review from ZNeumann and zsistla April 12, 2024 16:29

lavarou self-assigned this Apr 12, 2024

zsistla reviewed Apr 12, 2024

View reviewed changes

agent/php_execute.c Outdated Show resolved Hide resolved

lavarou added 3 commits April 16, 2024 11:27

simplify exception handler check in func_end

5c7f58d

There's no need to check if wraprec is not NULL - it is guaranteed to be not NULL at this point.

test with closure as user exception handler

1d080ab

improve testing of oapi exception handlers instrumentation

ceeb424

lavarou marked this pull request as ready for review April 16, 2024 17:24

lavarou added the ready for review label Apr 16, 2024

ZNeumann reviewed Apr 16, 2024

View reviewed changes

tests/integration/errors/test_oapi_uncaught_handled_exception_04.php Outdated Show resolved Hide resolved

ZNeumann reviewed Apr 16, 2024

View reviewed changes

tests/integration/errors/test_oapi_uncaught_handled_exception_04.php Outdated Show resolved Hide resolved

ZNeumann approved these changes Apr 16, 2024

View reviewed changes

cleanup tests of dead code

bd384cb

Co-authored-by: ZNeumann <[email protected]>

zsistla approved these changes Apr 18, 2024

View reviewed changes

improve test cases for PHP 8.3.x

4646c9c

PHP 8.3 introduced changes to handling exceptions and honoring exception handler stack. Add tests that illustrate those behavior differences.

zsistla reviewed Apr 18, 2024

View reviewed changes

lavarou added 2 commits April 18, 2024 17:46

improve test filenames

e8669dd

improve test filenames for PHP 8.3.x

5013256

Explain why different tests are needed in test description rather than test filename.

lavarou merged commit eecccd5 into dev Apr 19, 2024
62 checks passed

lavarou deleted the fix/agent/oapi-exception-handler-instrumentation branch April 19, 2024 13:19

lavarou mentioned this pull request Apr 19, 2024

feat(agent): add support for Yii v2 #848

Merged

lavarou added this to the next-release milestone Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(agent): improve exception handler instrumentation for PHPs 8.0+ #877

fix(agent): improve exception handler instrumentation for PHPs 8.0+ #877

lavarou commented Apr 12, 2024

newrelic-php-agent-bot commented Apr 12, 2024 •

edited

Loading

codecov-commenter commented Apr 12, 2024 •

edited

Loading

zsistla commented Apr 18, 2024

lavarou commented Apr 18, 2024

lavarou commented Apr 18, 2024

lavarou commented Apr 18, 2024

zsistla Apr 18, 2024

lavarou Apr 18, 2024

zsistla Apr 18, 2024

zsistla Apr 18, 2024

zsistla Apr 18, 2024 •

edited

Loading

lavarou Apr 18, 2024

zsistla commented Apr 18, 2024

lavarou commented Apr 18, 2024

lavarou commented Apr 18, 2024

zsistla commented Apr 18, 2024

fix(agent): improve exception handler instrumentation for PHPs 8.0+ #877

fix(agent): improve exception handler instrumentation for PHPs 8.0+ #877

Conversation

lavarou commented Apr 12, 2024

newrelic-php-agent-bot commented Apr 12, 2024 • edited Loading

codecov-commenter commented Apr 12, 2024 • edited Loading

Codecov Report

zsistla commented Apr 18, 2024

lavarou commented Apr 18, 2024

lavarou commented Apr 18, 2024

lavarou commented Apr 18, 2024

zsistla Apr 18, 2024

Choose a reason for hiding this comment

lavarou Apr 18, 2024

Choose a reason for hiding this comment

zsistla Apr 18, 2024

Choose a reason for hiding this comment

zsistla Apr 18, 2024

Choose a reason for hiding this comment

zsistla Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

lavarou Apr 18, 2024

Choose a reason for hiding this comment

zsistla commented Apr 18, 2024

lavarou commented Apr 18, 2024

lavarou commented Apr 18, 2024

zsistla commented Apr 18, 2024

newrelic-php-agent-bot commented Apr 12, 2024 •

edited

Loading

codecov-commenter commented Apr 12, 2024 •

edited

Loading

zsistla Apr 18, 2024 •

edited

Loading