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 using domain throttling, apply delay only when email was sent #968

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

bramley
Copy link
Contributor

@bramley bramley commented Jul 1, 2023

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
Copy link
Member

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
Copy link
Member

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

} elseif (MAILQUEUE_BATCH_SIZE && MAILQUEUE_AUTOTHROTTLE) {

@bramley
Copy link
Contributor Author

bramley commented Jul 28, 2023

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
Copy link
Member

michield commented Aug 9, 2023

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

@bramley
Copy link
Contributor Author

bramley commented Aug 10, 2023

Yes, I will revise the pull request.

Move delay processing to be within the $success branch
@bramley
Copy link
Contributor Author

bramley commented Aug 10, 2023

Now added another commit that moves the delay processing.

@michield
Copy link
Member

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

@michield michield self-requested a review August 12, 2023 09:02
@aulona1 aulona1 changed the base branch from main to release-3.6.14 September 13, 2023 15:09
@aulona1 aulona1 merged commit 0612625 into phpList:release-3.6.14 Sep 13, 2023
4 of 6 checks passed
@phpListDockerBot
Copy link
Contributor

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

@bramley bramley deleted the process_queue_delay branch September 18, 2023 22:26
@phpListDockerBot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants