1618088 - "Unexpected exception when instantiating class" during some operations

Status: RESOLVED FIXED

(Conduit :: Phabricator, defect, P2)

Description

[:glob ✱]

After upgrading our instance to week 2020/6, some of our revisions are no longer working in the differential.diff.search API:
It returns 500: ERR-CONDUIT-CORE, Unexpected exception when instantiating class.

Additionally, there's a few third-party tools in use at Mozilla to send patches to Phabricator.
As of the recent upgrade, they're failing to upload unless some additional parameters are added to the call (such as isMissingOldNewline and isMissingNewNewline).
It seems like diffs that are failing to load via differential.diff.search are those that were submitted with clients that didn't provide these previously-optional parameters.

To reproduce API error:

• Find a revision that was submitted using tools that doesn't provide the new required parameters
• Call the API:

Method: differential.diff.search
constraints: {"phids": ["PHID-DIFF-$id"]}
attachments: {"commits": true}

500: Unexpected exception when instantiating class.

Comment 1

[:glob ✱]

Phacility pointed to Sentry being the source of that particular error message:

The only piece of code I can find on the internet which includes this string is a PHP HTTP package:
https://github.com/php-http/discovery/blob/master/src/ClassDiscovery.php#L210
This is indirectly referenced by issues in Sentry:
https://github.com/getsentry/sentry-php/issues/795

Comment 2

[:glob ✱]

We're going to rollback the recent Phabricator upgrade which triggered the errors.

Comment 3

[:glob ✱]

Test case:

echo '{"constraints":{"phids":["PHID-DIFF-wowv355lg6eip4obii6r"]},"attachments":{"commits":true}}' | arc call-conduit --conduit-uri https://phabricator.services.mozilla.com/ differential.diff.search

Comment 4

[:glob ✱]

phabricator has been rolled back to the previous version.

we'll work on replicating this issue on stage.

Comment 5

[:glob ✱]

I've been playing around on phabricator-dev using a version of moz-phab which doesn't send isMissingOldNewline and isMissingNewNewline.

When trying to create a revision, if sentry is enabled we get the Unexpected exception when instantiating class error.
If sentry is disabled, the revision is created successfully. This is an unexpected lack of an exception.

Comment 6

[David Lawrence [:dkl]]

Attachment: full trace

Comment 7

[David Lawrence [:dkl]]

The content of attachment 9130901 [details] has been deleted

Comment 8

[David Lawrence [:dkl]]

NOTICE: PHP message: [2020-03-04 20:57:27] EXCEPTION: (Error) Call to undefined function blah() at [<moz-extensions>/conduit/FeedQueryIDConduitAPIMethod.php:33]
NOTICE: PHP message: arcanist(), moz-extensions(), phabricator(), phutil()
NOTICE: PHP message: #0 phlog(Error) called at [<phabricator>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
NOTICE: PHP message: #1 PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable(AphrontRequest, Error) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:751]
NOTICE: PHP message: #2 AphrontApplicationConfiguration::handleThrowable(Error) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:299]
NOTICE: PHP message: #3 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:207]
NOTICE: PHP message: #4 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:35]
NOTICE: PHP message: [2020-03-04 20:57:27] EXCEPTION: (PhutilAggregateException) Encountered a processing exception, then another exception when trying to build a response for the first exception.
- Http\Discovery\Exception\ClassInstantiationFailedException: Unexpected exception when instantiating class.
- Error: Call to undefined function blah() at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:319]
NOTICE: PHP message: arcanist(), moz-extensions(), phabricator(), phutil()
NOTICE: PHP message: #0 <#6> Http\Discovery\Exception\DiscoveryFailedException::create(array) called at [<phabricator>/externals/extensions/php-http/discovery/src/ClassDiscovery.php:79]
NOTICE: PHP message: #1 <#6> Http\Discovery\ClassDiscovery::findOneByType(string) called at [<phabricator>/externals/extensions/php-http/discovery/src/Psr17FactoryDiscovery.php:53]
NOTICE: PHP message: #2 <#5> Http\Discovery\Psr17FactoryDiscovery::createException(string, Http\Discovery\Exception\DiscoveryFailedException) called at [<phabricator>/externals/extensions/php-http/discovery/src/Psr17FactoryDiscovery.php:55]
NOTICE: PHP message: #3 <#5> Http\Discovery\Psr17FactoryDiscovery::findResponseFactory() called at [<phabricator>/externals/extensions/php-http/curl-client/src/Client.php:84]
NOTICE: PHP message: #4 <#5> Http\Client\Curl\Client::__construct() called at [<phabricator>/externals/extensions/php-http/discovery/src/ClassDiscovery.php:203]
NOTICE: PHP message: #5 <#4> FeedQueryIDConduitAPIMethod::execute(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/method/ConduitAPIMethod.php:123]
NOTICE: PHP message: #6 <#4> ConduitAPIMethod::executeMethod(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:131]
NOTICE: PHP message: #7 <#4> ConduitCall::executeMethod() called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:81]
NOTICE: PHP message: #8 <#4> ConduitCall::execute() called at [<phabricator>/src/applications/conduit/controller/PhabricatorConduitAPIController.php:83]
NOTICE: PHP message: #9 <#4> PhabricatorConduitAPIController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:287]
NOTICE: PHP message: #10 <#3> Http\Discovery\ClassDiscovery::instantiateClass(string) called at [<phabricator>/externals/extensions/php-http/discovery/src/HttpAsyncClientDiscovery.php:30]
NOTICE: PHP message: #11 <#3> Http\Discovery\HttpAsyncClientDiscovery::find() called at [<phabricator>/externals/extensions/sentry/sentry/src/HttpClient/HttpClientFactory.php:120]
NOTICE: PHP message: #12 <#3> Sentry\HttpClient\HttpClientFactory::create(Sentry\Options) called at [<phabricator>/externals/extensions/sentry/sentry/src/Transport/DefaultTransportFactory.php:50]
NOTICE: PHP message: #13 <#3> Sentry\Transport\DefaultTransportFactory::create(Sentry\Options) called at [<phabricator>/externals/extensions/sentry/sentry/src/ClientBuilder.php:291]
NOTICE: PHP message: #14 <#3> Sentry\ClientBuilder::createTransportInstance() called at [<phabricator>/externals/extensions/sentry/sentry/src/ClientBuilder.php:261]
NOTICE: PHP message: #15 <#3> Sentry\ClientBuilder::getClient() called at [<moz-extensions>/logging/SentryLoggerPlugin.php:103]
NOTICE: PHP message: #16 <#3> SentryLoggerPlugin::handleError(string, Error, array) called at [<phutil>/src/error/PhutilErrorHandler.php:428]
NOTICE: PHP message: #17 <#3> PhutilErrorHandler::dispatchErrorMessage(string, Error, array) called at [<phutil>/src/error/phlog.php:36]
NOTICE: PHP message: #18 <#3> phlog(Error) called at [<phabricator>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
NOTICE: PHP message: #19 <#3> PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable(AphrontRequest, Error) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:751]
NOTICE: PHP message: #20 <#3> AphrontApplicationConfiguration::handleThrowable(Error) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:299]
NOTICE: PHP message: #21 <#2> AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:207]
NOTICE: PHP message: #22 <#2> AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:35]
NOTICE: PHP message: #23 phlog(PhutilAggregateException) called at [<phabricator>/src/aphront/response/AphrontUnhandledExceptionResponse.php:32]
NOTICE: PHP message: #24 AphrontUnhandledExceptionResponse::setException(PhutilAggregateException) called at [<phabricator>/webroot/index.php:46]
NOTICE: PHP message: Error handler was reentered, some errors were not passed to the listener.
172.18.0.5 - 04/Mar/2020:20:57:27 +0000 "POST /index.php" 500
172.18.0.5 - 04/Mar/2020:20:57:27 +0000 "GET /index.php" 404

Comment 9

[Mitchell Hentges [:mhentges] 🦀]

Attachment: Bug 1618088 - installs a guzzle http interop layer for Sentry

Sentry depends on an HTTP implementation. They recently changed implementations from
zendframework/zend-diactoros to guzzlehttp/psr7 in their 2.3.0 release:
https://github.com/getsentry/sentry-php/releases/tag/2.3.0

However, the tool they use to detect HTTP implementations can't actually detect
guzzlehttp: https://github.com/php-http/discovery/issues/164

By providing the "http-interop/http-factory-guzzle" interop library between guzzlehttp
and the detection tool, we can ensure that Sentry is able to perform HTTP requests again.

Comment 10

[Mitchell Hentges [:mhentges] 🦀]

Attachment: Bug 1618088 - installs a guzzle http interop layer for Sentry

Sentry depends on an HTTP implementation. They recently changed implementations from
zendframework/zend-diactoros to guzzlehttp/psr7 in their 2.3.0 release:
https://github.com/getsentry/sentry-php/releases/tag/2.3.0

However, the tool they use to detect HTTP implementations can't actually detect
guzzlehttp: https://github.com/php-http/discovery/issues/164

By providing the "http-interop/http-factory-guzzle" interop library between guzzlehttp
and the detection tool, we can ensure that Sentry is able to perform HTTP requests again.

Comment 11

[Mitchell Hentges [:mhentges] 🦀]

TL;DR: I've created a Sentry ticket here, a patch for phabricator-extensions here

---

H'okay, we have two issues:

1. When Sentry tries to send exceptions, it's failing
2. For some reason, not providing isMissingOldNewline is triggering Sentry (which then fails). Weirdly, as glob found, if Sentry isn't active, then no exception is raised (so, why was Sentry being triggered? 🤔)

I haven't been able to investigate #2 yet - I don't have a good local reproduce.
But, I'm here to talk about #1.

Why does Sentry fail to send exceptions?

When Sentry tries to send an exception, it needs an HTTP implementation. To find one, it uses php-http/discovery, which has a hard-coded list of library implementations.
This hard-coded list does not work for guzzlehttp.

How did this use to work?

Sentry used to use zendframework/zend-diactoros, which php-http/discovery works with.
However, in release 2.3.0 (released early January, what a coincidence), Sentry changed to use guzzle, which now fails

Why are we affected by this? We didn't upgrade Sentry

We don't pin our Composer dependencies, we just install the most-recent versions, which exposes us to API breakage and "latest-version issues". On our Phabricator installations after the release of Sentry 2.3.0, Sentry didn't work anymore.
IMHO, we should pin our Composer dependencies.

What's next?

1. Land my patch
2. Figure out why isMissingOldNewline was triggering Sentry
3. Pin our composer deps using a composer.json and composer.lock file

Comment 12

[Mitchell Hentges [:mhentges] 🦀]

The content of attachment 9130941 [details] has been deleted for the following reason:

accidental dupe

Comment 13

[Mitchell Hentges [:mhentges] 🦀]

Attachment: Bug 1618088 - installs a guzzle http interop layer for Sentry

Sentry depends on an HTTP implementation. They recently changed implementations from
zendframework/zend-diactoros to guzzlehttp/psr7 in their 2.3.0 release:
https://github.com/getsentry/sentry-php/releases/tag/2.3.0

However, the tool they use to detect HTTP implementations can't actually detect
guzzlehttp: https://github.com/php-http/discovery/issues/164

By providing the "http-interop/http-factory-guzzle" interop library between guzzlehttp
and the detection tool, we can ensure that Sentry is able to perform HTTP requests again.

Comment 14

[Mitchell Hentges [:mhentges] 🦀]

Let's talk about point #2 from up above^:

2. For some reason, not providing isMissingOldNewline is triggering Sentry (which then fails). Weirdly, as glob found, if Sentry isn't active, then no exception is raised (so, why was Sentry being triggered? 🤔)

When Sentry wasn't active, why was no exception being raised?

This isn't actually true!
A PHP error was being raised when isMissingOldNewline isn't set.
However, Phabricator implements a custom global error handler that is invoked on interpreter errors such as undefined variables/properties/constants etc.
This error handler does two things:

1. Converts some PHP errors into exceptions (see the implementation here)
2. It invokes all "error listeners" (including Sentry) with the error information.

Here's the problem: though this custom error handler considers many invalid variable usages as exceptions (Failed typehint, undefined variables, undefined properties, undefined constants, any E_RECOVERABLE_ERROR), it does not convert undefined index into exceptions.

So, this has four repercussions:
1 . When isMissingOldNewline isn't available, the error is printed in server logs
2. When isMissingOldNewline isn't available, the error is reported to Sentry (here it is, reported since April 18, 2019!)
3. When isMissingOldNewline isn't available, the control flow resumes and pretends there was no error
4. If any "error listeners" throw an exception, then that failure is brought all the way to the top level and the request fails.

So, why has this problem only really affected us in the last couple weeks?

When our Sentry "error listener" was failing, then repercussion 4 ^ was encountered. Sentry was only failing in the last couple weeks.

What do we do?

1. Inform upstream that undefined index errors are ignored (this might be intentional)
2. We deploy the newer version of Phabricator, now that the Sentry issue is resolved

:glob, what do you think? Time to deploy?

Comment 15

[Mitchell Hentges [:mhentges] 🦀]

Upstream has responded, they may tighten their custom error handler to count undefined indexes as RuntimeException

Comment 16

[:glob ✱]

:glob, what do you think? Time to deploy?

great work! good to deploy.

Comment 17

[David Lawrence [:dkl]]

Can we close this since it is now deployed?

Comment 18

[Mitchell Hentges [:mhentges] 🦀]

Sounds good to me!