Apply other fix for #391 #576

Jean85 · 2018-03-21T14:12:57Z

This implements the regression suggested in #391 (comment) and should finally fix the issue with fatals + curl async.

Thanks again to @mfb for helping me debugging this issue.

stayallive

👍 looks good although Travis doesn't like it...

mfb · 2018-03-21T16:00:53Z

test/Raven/phpt/fatal_reported_with_async.phpt

@@ -21,7 +22,7 @@ $client->setSendCallback(function () {

 $client->install();

-register_shutdown_function(function () use ($pendingEvents, $pendingRequests) {
+register_shutdown_function(function () use (&$pendingEvents, &$pendingRequests) {


You are using a reference for &$pendingRequests but I think this is still pointing at the list of requests when the script starts rather than actual client state?

Nope, it's getting the actual client state at the end, since the test was failing until I applied 6eb54f2

I just double-checked and no, the test will pass even if I break the code such that there are still pending requests. $pendingRequests is a reference to the initial state of there being no pending requests, not the actual client's pending requests.

Damn you're right once again 😄

Ok, so is 01a26d3 enough to fix it? It seems to fail now if I comment stuff inside the client...

Jean85 · 2018-04-18T14:00:50Z

I've decided to avoid fixing this under PHP 5.4 and 5.5. Those are long gone EOLed versions, and the issue is located in the JSON serialization of the exception during fatal error handling, so it's nearly impossible to fix it now.

I think this PR should be merged anyway, so we at least have some value in all the other use cases, and we can bring these regression tests along into 2.x. Can you re-review it?

mfb · 2018-04-20T15:00:38Z

lib/Raven/Client.php

+        if (function_exists('mb_convert_encoding')) {
+            mb_convert_encoding('string', 'UTF8');
+        }
+        if (function_exists('mb_convert_encoding')) {


You have repeated these lines twice; was that intentional?

Also, what does mb_detect_encoding() and mb_convert_encoding() have to do with autoloading?

The fact that they are there twice is a mistake, I'll delete them ASAP.

As for the rest, functions are autoloaded too, so I need to trigger that in advance, to avoid triggering the autoload during the fatal error handling phase, where it would crash hard.

Oh interesting! Where can I read about autoload for functions? I thought you couldn't autoload functions..

Sorry, I used the wrong term. Functions are not "autoloaded" like classes, but still they are searched by namespace and loaded with a similar logic. So calling a function for the first time only during error handling makes everything crash under PHP 5, and that's why I'm forced to do this workaround and call them at least once before proceeding.

eesh! I had never seen a requirement to call functions first, but happily I don't use PHP 5 anymore so can't dig into this :)

mfb · 2018-04-20T15:02:13Z

lib/Raven/Client.php

@@ -195,6 +200,7 @@ public function __construct($options_or_dsn = null, $options = array())
        $this->context = new Raven_Context();
        $this->breadcrumbs = new Raven_Breadcrumbs();
        $this->_shutdown_function_has_been_set = false;
+        $this->useCompression = function_exists('gzcompress') && extension_loaded('zip');


Why does zip extension matter here? Zip extension doesn't provide gzcompress().

In older PHP versions (5.4 to 5.6, see this build of 68eb97c, which is the commit before ce2b5b5 which introduced this fix) the zip extension is not always present... I tried to not check the extension but it wasn't working.

mfb · 2018-04-20T15:11:32Z

lib/Raven/ErrorHandler.php

@@ -138,6 +138,8 @@ public function handleFatalError()
                @$error['message'], 0, @$error['type'],
                @$error['file'], @$error['line']
            );
+
+            $this->client->useCompression &= PHP_VERSION_ID > 70000;


Maybe it's just me, but using bitwise operator on boolean feels wrong to me :)

Wops! That works by mere coincidence I think 😅 fixing now!

mfb · 2018-04-20T15:12:29Z

test/Raven/Tests/ClientTest.php

@@ -2072,7 +2072,7 @@ public function testEncode()
        $this->assertRegExp('_^[a-zA-Z0-9/=]+$_', $value, 'Raven_Client::encode returned malformed data');
        $decoded = base64_decode($value);
        $this->assertInternalType('string', $decoded, 'Can not use base64 decode on the encoded blob');
-        if (function_exists("gzcompress")) {
+        if (function_exists('gzcompress') && extension_loaded('zip')) {


Another case where I think zip extension doesn't matter?

It's the same stuff, but on the other side, the test... I'll try to change this and see if it still works

Jean85 · 2018-04-20T15:25:16Z

Build is green, all is right! 🎉

Thanks for the multiple, very thorough reviews @mfb!

mfb · 2018-04-20T15:27:56Z

lib/Raven/ErrorHandler.php

@@ -138,6 +138,8 @@ public function handleFatalError()
                @$error['message'], 0, @$error['type'],
                @$error['file'], @$error['line']
            );
+
+            $this->client->useCompression = $this->client->useCompression && PHP_VERSION_ID > 70000;


Would be good to comment why PHP version matters here.

Well, now we have a regression test with this PR, so removing that check will make the test fail under PHP 5.

Basically, the reason is that under PHP 7 handling errors is no more a "special place" (thanks to \Throwable probably), so autoloading and other normal stuff work there too.

Jean85 added 2 commits March 21, 2018 15:10

Improve regression test for #575

b4ef866

Apply final fix for #575 and #391

806091e

Jean85 added the Type: Bug label Mar 21, 2018

Jean85 self-assigned this Mar 21, 2018

Jean85 requested review from stayallive, ste93cry and nokitakaze March 21, 2018 14:12

Jean85 mentioned this pull request Mar 21, 2018

Fatal errors are not recorded with the curl method async #391

Closed

stayallive approved these changes Mar 21, 2018

View reviewed changes

Jean85 added 4 commits March 21, 2018 15:42

Refactor PHPT test to avoid triggering autoloader during shutdown

325999f

Trigger early autoload for the gzcompress function

68eb97c

Disable zip compression while handling a fatal under PHP 5.x

ce2b5b5

Trigger other autoloads

63f834d

mfb reviewed Mar 21, 2018

View reviewed changes

Jean85 mentioned this pull request Mar 26, 2018

Fix x-forwarded-proto detection #578

Merged

Jean85 added 2 commits April 18, 2018 15:44

Avoid false failure in regression test

6eb54f2

Fix CS

3f60d53

Jean85 force-pushed the fix-575 branch from d02b21d to 3f60d53 Compare April 18, 2018 13:52

Ignore failures under PHP 5.4/5.5

c5f236c

Move inspection of client state inside the shutdown function

01a26d3

mfb reviewed Apr 20, 2018

View reviewed changes

Apply review fixes

621277a

mfb reviewed Apr 20, 2018

View reviewed changes

Jean85 merged commit 61c862b into master Apr 20, 2018

Jean85 deleted the fix-575 branch April 20, 2018 16:01

Jean85 mentioned this pull request May 3, 2018

[2.0] Merge up to 1.9 #592

Merged

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply other fix for #391 #576

Apply other fix for #391 #576

Jean85 commented Mar 21, 2018

stayallive left a comment •

edited

Loading

mfb Mar 21, 2018

Jean85 Apr 18, 2018

mfb Apr 18, 2018

Jean85 Apr 19, 2018

Jean85 commented Apr 18, 2018

mfb Apr 20, 2018

Jean85 Apr 20, 2018

mfb Apr 20, 2018

Jean85 Apr 20, 2018

mfb Apr 20, 2018

mfb Apr 20, 2018

Jean85 Apr 20, 2018

mfb Apr 20, 2018

Jean85 Apr 20, 2018

mfb Apr 20, 2018

Jean85 Apr 20, 2018

Jean85 commented Apr 20, 2018

mfb Apr 20, 2018

Jean85 Apr 20, 2018 •

edited

Loading

Apply other fix for #391 #576

Apply other fix for #391 #576

Conversation

Jean85 commented Mar 21, 2018

stayallive left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jean85 commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jean85 commented Apr 20, 2018

Choose a reason for hiding this comment

Jean85 Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

stayallive left a comment •

edited

Loading

Jean85 Apr 20, 2018 •

edited

Loading