Problem/motivation

  1. Currently RecipientHandlerInterface::buildRecipientQuery() is required to return an instance of \Drupal\Core\Database\Query\Select. In most cases the implementation will actually need to query entities, and the correct Drupal 8 way is to use \Drupal::entityQuery(). Querying entities via a select means writing much more complex code that breaks if alternative storage is used. Ideally we should allow the recipient handler complete flexibility and not force the use of SQL.
  2. The current design spools every recipient synchronously in response to a GUI action. If we use entity queries, then spooling will require loading each node. There could be 100k recipients, so in D8 we need to process them asynchronously in batches.
  3. The two stage spool/send leaves complexity/bugs with "window conditions". For example currently if a subscribed user changes their email address between spooling and send they will not receive the newsletter issue.

Proposed solution

Combine spool and send into a single phase. In response to the GUI just mark the issue as pending. During cron, loop over issues in the pending state. The RecipientHandler recieves an issue entity plus OFFSET and LIMIT and should return a list of MailInterface. Roughly like this:

  /**
   * Prepares a batch of mails for recipients.
   *
   * @param \Drupal\node\ContentEntityInterface $issue
   *   The newsletter issue being sent.
   * @param int $limit
   *   (Optional) The maximum number of mails to load from the spool. Defaults
   *   to unlimited.
   *
   * @return array
   *   Array of \Drupal\simplenews\Mail\MailInterface
   */
  function prepareMails(ContentEntityInterface $issue, $limit = self::UNLIMITED);

  /**
   * Tracks that a mail has been processed.
   *
   * Updates the newsletter issue entity so that any future call to
   * prepareMails() will exclude this mail.
   *
   * @param \Drupal\simplenews\Mail\MailInterface $mail
   *   The mail that has been processed.
   */
  function trackMail(MailInterface $mail);

Alternative solution

See #1931006: Problem with recipient handlers.

For discussion/comparison, see #13,#14.

Comments

AdamPS created an issue. See original summary.

adamps’s picture

adamps commented
Issue summary: View changes
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

Idea:

1) execute the query returned by buildRecipientQuery to convert it to an array, and then adjust the insert query to use that array

2) create a SimplenewsEntityQuery class that adds an addExpression method to EntityQuery. This sounds horrid, but is actually trivial as EntityQuery uses SelectQuery internally so it's a simple pass-through method.

3) tag the query and then alter it in a hook, per https://drupal.stackexchange.com/questions/218011/how-can-i-debug-entity...

4) use reflection setAccessible to make the internal selectquery in an entityquery public

adamps’s picture

adamps commented

Thanks for the ideas.

2-4) don't work. We cannot choose the class - we call \Drupal::database()->select() and Drupal chooses the class. If the site has chosen to use custom storage then it will be a different class that doesn't have an internal selectquery and maybe doesn't even use SQL. That's why the property is protected rather than public.

1) Yes I followed this logic myself and didn't yet get a chance to write it down here.

  • I don't it works for buildRecipientQuery to return an unspecified type that happens to have an execute method.
  • We could define it return entity query, but that doesn't work because executing an entity query only returns entity IDs - it cannot pass back field values.
  • We could fix it if we simply give RecipientHandler the flexibility and get it to return an array directly (hence not forced to use SQL). However this is likely to be unacceptable perf. The only way to get field values is to load each entity.

And my next thought was rather more radical. Let's entirely remove the Mail Spool - there's no need to enumerate the subscribers when the admin clicks "send" in the GUI - which is subject to GUI timeouts and forced to process the entire list in one go. Instead let's enumerate them during cron, each time processing a block of the limited size.

I'll write up some more soon.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

I don't see why #3.3 isn't viable:
Connection::select() returns an instance of \Drupal\Core\Database\Query\SelectInterface which extends AlterableInterface (as does EntityQuery) and has the AddTag method. So we can definitely add the tag.

What I don't know is whether hook_query_TAG_alter is passed the EntityQuery (in which case we can't add the expression in a hook) or the protected inner query (in which case we're golden). I'm guessing the latter.

Let's entirely remove the Mail Spool - there's no need to enumerate the subscribers when the admin clicks "send" in the GUI - Instead let's enumerate them during cron

Sounds like a job for a new queue operation. One job spools the mails from the newsletter, a second job processes the spool.

Of course this only addresses the GUI aspect, and is relatively easy. Getting rid of the spool would be quite a rewrite, and would need some complex logic to track progress across cron calls (they have a max execution time of their own) and handle send retries.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

We could define it return entity query, but that doesn't work because executing an entity query only returns entity IDs

Ouch. That really stumps me. Ignore my last comment ...

adamps’s picture

adamps commented

I found an old 7.x-2.x issue that is similar #1931006: Problem with recipient handlers and proposes adding a method on the recipient handler to spool entries - we can use that idea here. However the old issue doesn't cover the problem of entity query in D8, so we still need to think about that.

Sounds like a job for a new queue operation. One job spools the mails from the newsletter, a second job processes the spool.

I agree that would work. However it is a net increase in complexity.

Getting rid of the spool would be quite a rewrite, and would need some complex logic to track progress across cron calls

The logic isn't that hard if we just store the entity ID of the subscriber last processed. This would tie in with simplifying retries in #3052714: Send failures retried forever.

This feels like a bigger rewrite but a net decrease in complexity.

adamps’s picture

adamps commented

Sounds like a job for a new queue operation. One job spools the mails from the newsletter, a second job processes the spool.

Actually I'm not sure that's going to work. I don't think we can assume that it's acceptable to queue the entire newsletter in a single cron run, because in D8 it requires loading each node. So we could split the spooling into multiple batches with the complexity of keeping track of progress. However once we have done that and loaded the node, we might as well integrate the two parts and send the mails instead of just spooling.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

adding a method on the recipient handler to spool entries

I've been thinking the same thing:
Create a new SpoolingRecipientHandlerInterface with method addToSpool(query).
In spoolstorage addFromEntity() call that method if the handler implements the interface, else fall back to current logic for BC. Add this method to RecipientHandlerBase.

This would allow us to create an EntityQueryRecipientHandlerBase which extends RecipientHandlerBase but implements addToSpool() in a way that supports entity query for 99% of cases:
Check if the supplied query is an entity query and
use reflection to check for the protected inner query. If it's there get it, if not throw an error. Call the parent method passing the inner query.

This would work for almost everyone. Someone with a custom entity query would have to not create a custom plugin that extended this base class. But nobody breaks OOTB.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I'm only minimally following the current refactoring discussions, but make sure to test how it scales with large numbers of subscribers.

The currently solution is certainly not perfect and it is based on the 7.x-1.x -> 7.x-2.x refactoring that was never fully completed but it also his working on pretty large and complex installations as it is.

For example, the INSERT ... SELECT FROM statement by far outperforms anything that loads all the data and inserts it again and a batch might work in terms of memory usage but it will also make it much slower and error-prone (e.g., the current module can send with batch, but if aborted for any reason, it can pick up the remaining stuff on cron). And it's pretty easy to break a batch, users might close the tab, click on something, refresh the page...

adamps’s picture

adamps commented
Title: Use of hard-coded select instead of entityQuery » Problem with recipient handler interface and spooling
Issue summary: View changes
adamps’s picture

adamps commented

My previous comment and IS update is a cross-post with the two comments before.

@Berdir thanks for your comment. I know that we need to be careful about performance here and I understand the theory of what you are saying. However I would be very grateful if you could look in a little more detail to check that I haven't made a big mistake.

The key problem is that although INSERT ... SELECT FROM is very fast it's not really correct or easy to use with D8 Entity API.

The key proposed solution is that we no longer have a split of spool then send. There is no longer a mail spool! Clicking on send just marks the issue entity as pending. Anything else is too slow.

The sending phase in cron searches for issues that are pending. It calls the recipient handler which returns MailInterface (MailEntity) objects in 'bunches' which are then sent. When I said batch I don't mean batch as in the drupal UI mechanism I just mean we don't do them all at once but instead it can be split into multiple cron runs. At any point if PHP times out we can resume in the next cron job based on a subscriber ID (or other state) stored in the issue. Each subscriber is only loaded once, so it should be good perf.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I see. Some thoughts, has been a while since I did a simplenews project so might be missing more things:

One thing that the spool handles that I think isn't addressed yet with your approach is locking. We had a project where it was important to send out daily e-mails as fast as possible so everyone had them more or less at the same time. So we had a process on each application server that polled the spool in regular intervals and if there was something, it went into "burst mode" and sent as much as it could as fast as possible. That could be expanded with multiple threads and other fun stuff. For things like that to work, a lock process so that each recipient receives the newsletter exactly once is crucial. Maybe it would be possible to invert how the spool table works and insert that batch into the table on each run, combined with a table-level lock (lock spool table for write, fetch N records, insert them, free lock). But not quite sure yet how the "track by subscriber id" thing would work, that will also be critical to be process/thread-safe. That could then also still deal with failed e-mails (seen the other issue about endless retries being problematic, that's true, so we'll see where that goes).

There could also be issues with users receiving newsletters that were sent before the signed up.

One last thing, various features, specifically recipient handlers and non-node support in 7.x-2.x was pushed by the people behind the party CRM: https://www.drupal.org/project/party_simplenews. Was long ago, but IIRC, one thing they built on top of that is the ability to dynamically build a recipient list based on a view, so you could filter it as you wish and then send that out as a newsletter. Sounds like that might be challenging without the spool as it is now, as you'd essentially need to persist the view state and batch through that? Of course that coudn't use the INSERT ... SELECT FROM either, but that is basically optionally and you can use slower/different approaches to fill the spool. Looks like they are working on a new project for D8 (https://www.drupal.org/project/contacts), so it might be worthwhile to check if they have plans for Simplenews in 8.x?

adamps’s picture

adamps commented
Issue summary: View changes

1. Yes I agree we need locking.

I had imagined it would be similar to the current code: hook_cron will use a critical section to change the newsletter_issue to a new state LOCKED. Subsequent calls to hook_cron will only select PENDING issues except locks can be broken after a timeout. However relating to your example, I now see that this doesn't allow multiple threads/processes/servers to process the same issue.

2. True, the timing will change. We will send to the list of recipients based on when cron runs rather than when the send button was clicked. If the recipients are defined by a view then we would not attempt to save the view state, but rather use whatever the view state is when cron runs.

This could be seen as bad, but actually I think it's mostly good. For example, if someone has "unsubscribed" (or more subtly they altered some setting on their account that means they are no longer a recipient) we won't sent them more mails - whereas with an overloaded mail spool they could continue to receive mails for days afterwards.

Ah but I think you mean that the admin sets exposed filters then sends based on those. Tricky!

===

If it is too risky/limiting to drop the mail spool then I the main alternative I can see is to go back to something like #1931006: Problem with recipient handlers.

  • The default handler can 'cheat' and use INSERT ... SELECT FROM with a known limitation that the subscriber table mustn't have custom storage.
  • Other handlers can use EntityQuery or anything they like but with a pretty strict upper limit on number of recipients.
  • No obvious way to purge "unsubscribed" recipients from the spool in the case where they modified some account setting.
adamps’s picture

adamps commented

Thanks @Berdir. I wanted to be sure I wrote down my latest thoughts in the various issues.

Practically speaking, it's now clear to me that I don't have time on my current project to fix all the problems I've found. I will need to think about how to alter the design and which issues are essential, which not. It means that many of the issues are not going to see patches from me soon.

adamps’s picture

adamps commented

OK I am now in favour of the approach in #1931006: Problem with recipient handlers. We can optimise by removing the mail field from the spool and just relying on snid. That means that it will often be possible to use an entity query to return the snid values, without the need to load all the nodes to find the value of the mail field. As a bonus this solves a current bug that subscriber lookup fails because a user changed their mail.

It does mean we would lose #3054693: Allow mail spool entries that are not subscriber entity. However it's easy to create a subscriber for every user, which likely covers 90% of cases.

adamps’s picture

adamps commented
Status: Active » Closed (duplicate)

Closed in favour of #1931006: Problem with recipient handlers.