Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make queueing error mails configureable #32

Closed
wants to merge 7 commits into from
Closed

Make queueing error mails configureable #32

wants to merge 7 commits into from

Conversation

codegain
Copy link

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
Copy link
Contributor

@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
Copy link
Author

codegain commented Mar 2, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants