search

symfony / swiftmailer-bundle

Symfony Swiftmailer Bundle
https://symfony.com/swiftmailer-bundle
MIT License
1.56k stars 151 forks source link

Add concurrency lock to SendEmailCommand #259

Closed azine closed 4 years ago

azine commented 6 years ago

Only allow one instance of the SendEmailCommand to run at the same time to avoid concurrency issues with the DiskKeyCache in the SwiftMailer.

See #210 & #209

TL:DR: when running this command every minute with a cron-job and sending a big load of emails out at once, an error can occur when a second instance of this command is started one minute later, while the first one is still running. They both try to send the same mail => try to create the same temp directory => one of the instances fails and throws an exception.

dmaicher commented 5 years ago

What if you are running jobs on multiple servers and need a distributed lock for example?

The type of lock is really something that needs to be configurable imho and then its easily out of the scope of this bundle.

azine commented 5 years ago

Hi @dmaicher

hmmm.. so you say rather than having a locking mechanism that does the job if only one server is used and does not have a negative impact when multiple servers are used, you rather don't have a solution at all?

I don't see where we would loose anything by adding this code.

If you have a farm of 10 servers running your site. You would still not want that the send-email-job is executed twice on the same machine at the same time. ....you still have to solve the concurrency-accross-server-issue, but at least the concurrency-on-same-server-issue is solved.

please reconsider, approve and anyone with write-permissions, merge .... or give good reasons not to merge.... I am always happy to learn. :-)

dmaicher commented 5 years ago

I just think adding one specific lock that only solves one use-case is not enough. Then it must be documented well that other use-cases are still not fixed out-of-the box. And maybe then it will cause confusion.

Also not sure this file-system lock will actually work nicely in read-only file-systems and docker containers for example. So at least the directory used for the lock should be configurable?

So its something that must be properly planned & decided on.

Note: this is just my opinion :wink: I cannot decide anything here. It's up to the maintainers of this bundle.

azine commented 5 years ago

Thanks a lot for your comments. I see your point now.

fabpot commented 4 years ago

Closing as we won't add any new "big" features to this bundle in favor of Symfony Mailer.