anlutro / laravel-4-smart-errors

Smarter error handling for Laravel 4
65 stars 13 forks source link

if exception during bootstrap sequence stop smart error to be stuck in infinite loop #11

Closed m4nuC closed 9 years ago

m4nuC commented 10 years ago

Lets' say that you add a new composer package that you use in the application but do not composer install it. Laravel throws an error on bootstrap in such case smart error get stuck in an infinite loop and start spamming the email box with the same error over and over again. There should be a way to avoid such problem and only send one error.

anlutro commented 10 years ago

Hm. Why would it get stuck in an infinite loop?

m4nuC commented 10 years ago

Well it needs to bootstrap the framework and if an exception is thrown while bootstrapping then it get stuck in an infinite loop. Again as per the example I gave you, you can try to create a service provider that reference a class that does not exist. And you can see the problem by yourself.

anlutro commented 10 years ago

The error handler cannot cause an infinite loop like this. The framework is booted once, at which point the error handler is registered. If an error is thrown after the error handler is registered the whole application should short-circuit.

What is the exact error you're getting in your application that's causing the infinite loop? A service provider class not found error?

m4nuC commented 10 years ago

ReflectionException class not found.

Class referenced in the IOC did not exist as I didn't "composer install". I received almost a thousand email in 5 minutes with the same error.

anlutro commented 10 years ago

Where are you trying to reference the class? What class is it? I can't reproduce the issue.

m4nuC commented 10 years ago

I have a service provider that goes like this:

    class NewslettersServiceProvider extends ServiceProvider {
        /**
        * Register the binding
        *
        * @return void
        */
     public function register()
        {
            $this->app->bind('Nota\Services\Newsletters\NewslettersInterface', 'Nota\Services\Newsletters\Mailchimp\MailchimpNewsletters');
        }
    }

then in the MailchimpNewsletters class i reference the Mailchimp package. I tried to hit the app before having pulled that package with composer.

anlutro commented 10 years ago

So you inject an instance of the interface via type hinting into a controller, I imagine?

m4nuC commented 10 years ago

No that class is resolved in an event handler via app->make

anlutro commented 10 years ago

I still can't make sense of this. Where is your event being fired? Is there any queue logic involved in your application?

m4nuC commented 10 years ago

Well there are queues but they are not involved in this problem. I think it comes from the fact that the IOC refers to a class that doesn't exist and throw an exception. Since smart error also uses the IOC (i am assuming it does) that create an endless loop. Did you try to create a service provider that tries to bind a non existing class? I think that should be enough to replicate the issue.

anlutro commented 10 years ago

Just because one class can't be resolved from the IoC container does not mean the entire IoC container breaks, and even if it did, you wouldn't get an infinite loop - at worst you'd get an "Error in exception handler" message.

m4nuC commented 10 years ago

well it throws a ReflectionException which is not caught and handed to smart error witch itself uses the IoC which throws another ReflectionException and so on ..

anlutro commented 10 years ago

If the code inside the error handler causes a ReflectionException, the script will die and you'll see "Error in exception handler". See the following code:

https://github.com/laravel/framework/blob/v4.2.1/src/Illuminate/Exception/Handler.php#L247-L257 https://github.com/laravel/framework/blob/v4.2.1/src/Illuminate/Exception/Handler.php#L312-L328

Given the information you've given me, this is the closest I've come to recreating your environment: https://gist.github.com/anlutro/55fee32811a00cae5dce Which works just fine for me.

m4nuC commented 10 years ago

This would be closer https://gist.github.com/m4nuC/bead34ef5b294be3816f.

Although the binding are registered in a service provider which might, somehow be the cause of the problem, as the even handler did not get hit when bug appeared. FYI the handler is triggered only when a user registers and in my test i just tried to hit the homepage. Actually also tried artisan command which yield the same result

m4nuC commented 10 years ago

I finished receiving all emails. i got 573 with the same : ReflectionException class not found. Mailchimp not found error. My new relic graphs are not happy : (

anlutro commented 10 years ago

I will add a feature that prevents the exact same exception from sending a mail more than once, but I still think you have a problem in your application that needs to be fixed as this recursive thing cannot possibly be caused by my library.

m4nuC commented 10 years ago

Likely not. Its very simple code. Done it a 100 times. Thanks tho!

On Jun 2, 2014 6:40 PM, Andreas Lutro notifications@github.com wrote: I will add a feature that prevents the exact same exception from sending a mail more than once, but I still think you have a problem in your application that needs to be fixed as this recursive thing cannot possibly be caused by my library.


Reply to this email directly or view it on GitHub: https://github.com/anlutro/laravel-4-smart-errors/issues/11#issuecomment-44823062

anlutro commented 10 years ago

I've pushed the change as 2.3.2 - it should obviously stop the same exception from being emailed more than once, but I'd be interested to know if the error is still logged in your log file 500+ times after upgrading.

m4nuC commented 10 years ago

Well it's fixed on the prod app as I installed the missing dep but I ll try to reproduce that on a fresh app when I find a minute. Thanks for the fix.

On Jun 2, 2014 6:54 PM, Andreas Lutro notifications@github.com wrote: I've pushed the change as 2.3.2 - it should obviously stop the same exception from being emailed more than once, but I'd be interested to know if the error is still logged in your log file 500+ times after upgrading.


Reply to this email directly or view it on GitHub: https://github.com/anlutro/laravel-4-smart-errors/issues/11#issuecomment-44824019