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

When mails fail to send, queue them for later. #4782

Closed
icarito opened this issue Feb 9, 2019 · 15 comments · Fixed by #4967
Closed

When mails fail to send, queue them for later. #4782

icarito opened this issue Feb 9, 2019 · 15 comments · Fixed by #4967
Labels
enhancement explains that the issue is to improve upon one of our existing features help wanted requires help by anyone willing to contribute

Comments

@icarito
Copy link
Member

icarito commented Feb 9, 2019

Mail systems can sometimes have interruptions (e.g. we have a throttle from google for example).

Could we queue up failed outgoing emails?

There are subtle bugs like #4774 (comment) that are confusing and if we had a proper mail queue cache we would avoid them.

One non-coding way to do this is to implement an MTA container and configure it for such retry queue.
Or it can be done at the application level. MTAs are designed and for this task, but on the other hand this feature would only be pre-configured when running with Docker if we went the container path.

@grvsachdeva grvsachdeva added the enhancement explains that the issue is to improve upon one of our existing features label Feb 9, 2019
@dewanhimanshu
Copy link
Collaborator

We can use redis and resque for this . Resque also provide a web interface for admin.

@grvsachdeva
Copy link
Member

Actually, we already have redis and sidekiq setup so how about using them? Thanks!

@dewanhimanshu
Copy link
Collaborator

Thats great . yes we can use sidekiq also for this . Is there a web ui for it also?

@grvsachdeva
Copy link
Member

grvsachdeva commented Feb 13, 2019 via email

@dewanhimanshu
Copy link
Collaborator

Thats great . Then its better to use sidekiq and redis . We can create a background jobs/thread for the mails .

@grvsachdeva
Copy link
Member

Yup. you want to work on this @dewanhimanshu? Also, what do you think about this @icarito @jywarren? By queuing email for later, the user will not experience any delay or conflict in events like Signup, liking a comment, commenting, etc wherever email is sent in the background. Thanks!

@dewanhimanshu
Copy link
Collaborator

I will be difficult for me to work as my mid sem exams are comming . But thanks @gauravano for giving me this opportunity. User experience will remain smooth . Actually i have used background jobs in my projects and it improves the user experience . Mail will be send on seperate thread instead on main server thread.

@grvsachdeva
Copy link
Member

Yeah, I have also built a project in past using Redis and Resque, the user experience really improves with using queue. We can start this project after a week or two(I also have mid sem from next weekend 🙈). Let's work on this one in March or if someone wants to implement this, please go ahead.
And, all the best @dewanhimanshu for your exams!
Thanks!

@dewanhimanshu
Copy link
Collaborator

Thank you @gauravano . All the best for your exams also.

@grvsachdeva
Copy link
Member

Thank you!

@jywarren
Copy link
Member

jywarren commented Mar 9, 2019

Ah, it looks like we can use deliver_later in Rails now: https://guides.rubyonrails.org/action_mailer_basics.html#calling-the-mailer

Active Job's default behavior is to execute jobs via the :async adapter. So, you can use deliver_later now to send emails asynchronously. Active Job's default adapter runs jobs with an in-process thread pool. It's well-suited for the development/test environments, since it doesn't require any external infrastructure, but it's a poor fit for production since it drops pending jobs on restart. If you need a persistent backend, you will need to use an Active Job adapter that has a persistent backend (Sidekiq, Resque, etc).

I believe we are already set up with all this!

It's worth trying -- we can try to change any instance of deliver_now to deliver_later -- try one of these! https://github.com/publiclab/plots2/search?q=deliver_now

We'd love help with this if anyone's interested!

@jywarren jywarren added help wanted requires help by anyone willing to contribute and removed discussion labels Mar 9, 2019
@jywarren
Copy link
Member

jywarren commented Mar 9, 2019

Because the default runner in ActiveJob is inline, i wonder if perhaps it won't even break the tests! https://weblog.rubyonrails.org/2014/8/20/Rails-4-2-beta1/

@jywarren
Copy link
Member

jywarren commented Mar 9, 2019

Well, here's an initial test for all admin mailings: #4967

@jywarren
Copy link
Member

This worked! #4967 It required wrapping tests in a block starting with perform_enqueued_jobs do and end.

We could merge this now, then repeat for remaining instances of deliver_now:

https://github.com/publiclab/plots2/search?q=deliver_now

@jywarren
Copy link
Member

I think we should be good to merge this, since it only affects admin emails. We can try it out before reapplying this to other email systems. I've opened a new issue to repeat this process in other parts of the codebase:

#5311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement explains that the issue is to improve upon one of our existing features help wanted requires help by anyone willing to contribute
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants