squareboat / sneaker

An easy way to send emails whenever an exception occurs on server.
MIT License
222 stars 46 forks source link

Make queueing error mails configureable #32

Closed codegain closed 3 years ago

codegain commented 5 years ago

This PR (idea from #31) adds a new configuration option "should_queue" in the sneaker.php configuration file which determines if the error mail should be sent via the queue or not.

akaamitgupta commented 5 years ago

@codegain Thanks for the PR but still don't think it will resolve the issue #31 .

The new flow will be like

  1. Exception is thrown
  2. The exception is captured by Sneaker
  3. Sneaker generates an error mail and sends it immediately
  4. Sending mail fails, because the SMTP server responded with an error
  5. It throws an exception
  6. Loop to 1.

Your input on this?

codegain commented 5 years ago

I think the problem of #31 is that an exception is thrown inside the laravel mailer where the smtp connection is established if a worker sends out the error mail.

In the current version, the Mailable "ExceptionMailer" implements the interface "ShouldQueue" which will always queue the mail, even if it is sent via $mailer->send(...):

The process is as follows:

Sneaker.php $this->mailer->to($recipients)->send(new ExceptionMailer($subject, $body));

Mailer.php (Illuminate):

    public function send($view, array $data = [], $callback = null)
    {
        if ($view instanceof MailableContract) {
            return $this->sendMailable($view);
        }

which calls:

    protected function sendMailable(MailableContract $mailable)
    {
        return $mailable instanceof ShouldQueue
            ? $mailable->queue($this->queue) : $mailable->send($this);
    }

which currently always results in the mailable being queued if a queue is available. Therefore, none of the currently implemented try-catch blocks will catch any exception because the mail is successfully pushed onto the queue.

The worker will then execute the queued mail via the "SendQueuedMailable" job class:

    public function handle(MailerContract $mailer)
    {
        $this->mailable->send($mailer);
    }

which results in a call to ExceptionMailer->send() which is not overwritten and therefore the following code gets executed:

    public function send(MailerContract $mailer)
    {
        $this->withLocale($this->locale, function () use ($mailer) {
            Container::getInstance()->call([$this, 'build']);

            $mailer->send($this->buildView(), $this->buildViewData(), function ($message) {
                $this->buildFrom($message)
                     ->buildRecipients($message)
                     ->buildSubject($message)
                     ->runCallbacks($message)
                     ->buildAttachments($message);
            });
        });
    }

It is this $mailer->send(...) call which triggers another exception because the smtp connection could not be established (or something else) and this results in another $sneaker->capture() call.

Your input on this?

From my perspective, there are two options:

  1. Drop queueing of error mails altogether
  2. Overwrite the Mailable->send() Method in your ExceptionMailer and wrap the parent call into an try-catch block which catches any exception while sending the error mail.

@akaamitgupta I think option 2 is a good way to deal with this problem and I can submit another PR if you want.