anymail / django-anymail

Django email backends and webhooks for Amazon SES, Brevo (Sendinblue), MailerSend, Mailgun, Mailjet, Postmark, Postal, Resend, SendGrid, SparkPost, Unisender Go and more
https://anymail.dev
BSD 3-Clause "New" or "Revised" License
1.67k stars 129 forks source link

Support requests session customization, document use for automatic retries #277

Closed dgilmanAIDENTIFIED closed 2 years ago

dgilmanAIDENTIFIED commented 2 years ago

The requests library lets you mount adapters to configure policy such as timeouts and retries. Here's an attempt at an API that extends anymail to let you do so. This is the initial draft, I am looking forward to feedback on how this should be done. I also skipped writing docs.

The functionality is exposed through a new ANYMAIL_REQUESTS_ADAPTERS configuration value in the Django settings:

ANYMAIL_REQUESTS_ADAPTERS = [
    ["https://foobar.com", HTTPAdapter(pool_connections=100)],
]
medmunds commented 2 years ago

@dgilmanAIDENTIFIED cool, thanks for this! You've integrated it nicely into Anymail.

My one concern is whether there are a bunch of other requests.Session options we'd want to expose in a similar way. E.g., cert for a custom client certificate, or proxies. (And if so, what happens in future requests releases?) I'd like to avoid Anymail having a lot of requests-specific settings.

A more general approach could be a "session creation hook", which Anymail would call with the new session. Clients could do anything they want to that session, including mounting transport adapters. I'd probably lean toward Django's signal receiver mechanism to implement the hook (Anymail already uses it for some other hook-type features), but a setting could work, too.

Or one other way to modify the requests.Session—and this works in the current Anymail—is subclassing. You can subclass any Anymail requests-based backend to modify the session after it's created (or even create the session in a completely different way), and just set Django's EMAIL_BACKEND to your subclass. Right now, you have to override AnymailRequestsBackend.open(), which is a little ugly. It would probably be good to extract an AnymailRequestsBackend.create_session() method with base implementation return requests.Session(), just to facilitate this sort of override.

dgilmanAIDENTIFIED commented 2 years ago

Those are all good ideas. If you can flip a coin or whatever and pick one I'll give it a shot. I think any of these are good ways forward as long as they're documented.

medmunds commented 2 years ago

I don't have a strong preference, so if one seems like it would be a better fit for your use case, definitely go with that. (Are you doing bulk async sending where a large pool_connections helps?)

If you don't have a strong preference either, then I'd say let's start with extracting AnymailRequestsBackend.create_session() to simplify subclassing. It feels like that's useful either way. (And would probably be where we'd call a session-creation hook if we go that route.)

I've been meaning to add a docs section about what to do when Anymail does almost what you want, but not quite. And I thought it should mention subclassing a backend (people don't always realize EMAIL_BACKEND can point at their own code)—but didn't have a good example where that would be useful. You've got a good example. If you're interested in taking a crack at the docs, I'd be thrilled.

dgilmanAIDENTIFIED commented 2 years ago

My big use case is automatic retries on network errors. I was planning on documenting it on the one doc page that mentions how to catch network errors and retries in a background task.

medmunds commented 2 years ago

Looks great. Thank you for the contribution!