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

Out of memory when sending large disk attachments #51620

Closed
bytestream opened this issue May 29, 2024 · 5 comments
Closed

Out of memory when sending large disk attachments #51620

bytestream opened this issue May 29, 2024 · 5 comments

Comments

@bytestream
Copy link
Contributor

bytestream commented May 29, 2024

Laravel Version

10.48.12

PHP Version

8.2.19

Database Driver & Version

No response

Description

Sending a mailable with a large disk attachment results in an out of memory exception. This is because the whole file is loaded into memory at

$storage->get($attachment['path']),

It should be sent to Symfony mailer as a stream, which is already supported

* @param string|resource $data

Steps To Reproduce

/**
 * Get the attachments for the message.
 *
 * @return array<int, \Illuminate\Mail\Mailables\Attachment>
 */
public function attachments(): array
{
    return [
        Attachment::fromStorage('/path/to/large.pdf')
                ->as('large.pdf')
                ->withMime('application/pdf'),
    ];
}

https://laravel.com/docs/11.x/mail#attaching-files-from-disk

@driesvints
Copy link
Member

Thanks. Right now this isn't possible. It's also a bit unclear to me what you mean with "large"? How large are we talking about?

@bytestream
Copy link
Contributor Author

Thanks for getting back to me.

Right now this isn't possible.

It's possible to use streams rather than load the file into memory i.e. $storage->get($attachment['path']), becomes $storage->readStream($attachment['path']),. That ensures Symfony mailer reads the file in chunks rather than all at once.

It's also a bit unclear to me what you mean with "large"? How large are we talking about?

The size of the file doesn't really matter if you alter PHP's memory_limit directive i.e. php -d memory_limit=32M. The simple fact is that $storage->get($attachment['path']) unnecessarily loads the whole file into memory when there are better alternatives available.

@driesvints
Copy link
Member

I do believe size matters as I don't think it's reasonable to send too large attachments over mail. I think it's best that if you feel something can be improved here that you attempt to send in a PR. If it can be without BC breaks it can adjust the current functionality otherwise it should be a new method. Thanks.

@bytestream
Copy link
Contributor Author

@driesvints I agree that one shouldn't be sending large files over mail, but the code should be memory efficient and not enforce such limitations in an ungraceful manner. I sent a PR but it was closed without explanation #51601. Perhaps you can offer some insight given it doesn't break BC, and just improves the efficiency of the current functionality.

@driesvints
Copy link
Member

Hey @bytestream. It seems Taylor doesn't want to make that change right now. Since we only received one report for this we're going to leave things be sorry. You could always try to overwrite the buildDiskAttachments method if you want.

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

No branches or pull requests

2 participants