phpList / phplist3

Fully functional Open Source email marketing manager for creating, sending, integrating, and analysing email campaigns and newsletters.
https://www.phplist.org
GNU Affero General Public License v3.0
737 stars 268 forks source link

When using domain throttling, apply delay only when email was sent #968

Closed bramley closed 12 months ago

bramley commented 1 year ago

Description

When using domain throttling, if sending to an email address is rejected because the limit on its domain has been reached and an email was not sent, the delay of MAILQUEUE_THROTTLE is still applied. This seems unnecessary and slows down the sending rate.

The first commit is to apply MAILQUEUE_THROTTLE and MAILQUEUE_AUTOTHROTTLE only when the domain was not throttled. I don't fully understand the $running_throttle_delay processing so have left that as it is.

The second commit is to simplify the initialisation of the $domainthrottle array into one place, when a new domain is met or a new interval is started for an existing domain.

Related Issue

Screenshots (if appropriate):

michield commented 1 year ago

When DOMAIN_AUTO_THROTTLE is enabled, running_throttle_delay is adjusted when needed. The main objective is to spread the sending of DOMAIN_BATCH_SIZE over the total period of DOMAIN_BATCH_PERIOD, instead of sending DOMAIN_BATCH_SIZE in the first few seconds.

michield commented 1 year ago

The PR diff is a bit hard to read and see the consequences of.

Can we not just add "&& $succces" to

https://github.com/phpList/phplist3/blob/32094348a355b768ec01ed3b22ef09b3ccd5e9f1/public_html/lists/admin/actions/processqueue.php#L1220C52-L1220C52

and

https://github.com/phpList/phplist3/blob/32094348a355b768ec01ed3b22ef09b3ccd5e9f1/public_html/lists/admin/actions/processqueue.php#L1222

bramley commented 1 year ago

That would probably have the same effect. Maybe then move this whole chunk

                    if (isset($running_throttle_delay)) {
                        sleep($running_throttle_delay);
                        if ($counters['sent'] % 5 == 0) {
                            // retry running faster after some more messages, to see if that helps
                            unset($running_throttle_delay);
                        }
                    } elseif (MAILQUEUE_THROTTLE) {
                        usleep(MAILQUEUE_THROTTLE * 1000000);

...
                            }
                            usleep($delay * 1000000);
                        }
                    }

to be within this near line 1173, if it should be run only when the sending was successful and not when throttled or sending was unsuccessful.

if ($success) {

michield commented 1 year ago

Yes, that makes more sense. Do you want to update the PR to do that?

bramley commented 1 year ago

Yes, I will revise the pull request.

bramley commented 1 year ago

Now added another commit that moves the delay processing.

michield commented 1 year ago

It's a bit hard to test this, but I think it looks fine, so approving.

phpListDockerBot commented 11 months ago

This pull request has been mentioned on phpList Discuss. There might be relevant details there:

https://discuss.phplist.org/t/3-6-14-release-candidate-ready-for-testing/9109/1

phpListDockerBot commented 11 months ago

This pull request has been mentioned on phpList Discuss. There might be relevant details there:

https://discuss.phplist.org/t/phplist-3-6-14-released-security-release/9158/1