[feature/mass-email-speed-control]

[peterkclee]

I have implemented a feature to control the mass email speed.
Below is the link for my branch on github:
http://github.com/peterkclee/phpbb3/tre ... ed-control

Before adding this feature, the mass email speed (using queue) had no speed control.
Package size for email and jabber message could only limit the number of emails or messages sent when "cron.php" is run.
However, "cron.php" may run many times within one hour, meaning that any package size setting could make the sending speed too fast, exceeding the allowed speed for most shared web hosting environment.
This feature would control the mass email speed (using queue).
Code was also changed to use flock() to create critical section.
This would enable calling the queue related functions from codes other than "cron.php" without conflict.

[Oleg]

I did some work on improving queue locking here: http://tracker.phpbb.com/browse/PHPBB3-9061

Your patch mixes speed control and locking changes together, making it hard to figure out what's what.

On the actual patch:

1. Why all the (int) casts? Surely most if not all of them are useless.

2. The number 50 should probably be configurable. If someone's host allows them to send 250 emails every hour being limited to 50 emails would likely be painful.

3. You removed this text from documentation strings: "If set to 0 the message is sent immediately and will not be queued for later sending." I'm assuming you did not actually change this behavior, the note that package size of 0 results in immediate mail submission needs to go back in the docs.

4. How about adding an explanation how $process_history works in the code itself?

5. $n is not a descriptive variable name. Together with #4 it makes the code hard to figure out: no comments and variable name that is not self-explanatory.

Github cuts the diff off horizontally, how annoying.

[nickvergessen]

3. You removed this text from documentation strings: "If set to 0 the message is sent immediately and will not be queued for later sending." I'm assuming you did not actually change this behavior, the note that package size of 0 results in immediate mail submission needs to go back in the docs.

Code: Select all

+  'JAB_PACKAGE_SIZE_EXPLAIN'  => 'This setting is applied to the internal message queue. It is the maximum number of messages sent out in one package, as well as the maximum total number of messages sent out in multiple packages within one hour. Each message could have up to 50 recipients. If it is set to 0, all messages would be sent out in one package.',

It's at the end of the loooong line

[Oleg]

Apologies, I should clearly have looked at the patch in its entirety.

[Nelsaidi]

// comment removed

Sounds like a good idea for people who are on shared hosting.

[Oleg]

After reading the whole patch I have two more comments:

1. Locking changes in this patch are dangerous. if a lock cannot be obtained for any reason, outgoing mail will be silently dropped.

2.

If it is set to 0, all messages would be sent out in one package.

Yes, but also email will be sent immediately and not go thorough messenger queue. This part needs to go back in the option explanations.

[peterkclee]

I did some work on improving queue locking here: http://tracker.phpbb.com/browse/PHPBB3-9061

Your patch mixes speed control and locking changes together, making it hard to figure out what's what.

On the actual patch:

1. Why all the (int) casts? Surely most if not all of them are useless.

2. The number 50 should probably be configurable. If someone's host allows them to send 250 emails every hour being limited to 50 emails would likely be painful.

3. You removed this text from documentation strings: "If set to 0 the message is sent immediately and will not be queued for later sending." I'm assuming you did not actually change this behavior, the note that package size of 0 results in immediate mail submission needs to go back in the docs.

4. How about adding an explanation how $process_history works in the code itself?

5. $n is not a descriptive variable name. Together with #4 it makes the code hard to figure out: no comments and variable name that is not self-explanatory.

Github cuts the diff off horizontally, how annoying.

I think flock() is the only method to create critical section other than using database.

1. By investigating the cache/queue file, I found $package_size was originally stored as a string but not as an integer (which I expected). Perhaps it is ok, but it may create other kinds of problems which are difficult to trace (in the future). In fact, I think it is not easy to trace problems related to unexpected variable type when writing PHP program, so I always do type casting (for scalar types) in all variable assignment statements for my PHP programs.
2. The number 50 is hard coded as $max_chunk_size in “includes/acp/acp_email.php”.
3. The language file explains that already.
4. The variable $process_history / $this->queue_data[$object]['process_history’] is just an array using time as key and $num_items as value which is very easy to find in the code. I think it is self-explanatory compared to $this->queue_data[$object]['data'].
5. $n is just a temporary variable, just like using $i in most for-loop. In fact, I had tried to think a better name, but it is difficult.

[peterkclee]

After reading the whole patch I have two more comments:

1. Locking changes in this patch are dangerous. if a lock cannot be obtained for any reason, outgoing mail will be silently dropped.

2.

If it is set to 0, all messages would be sent out in one package.

Yes, but also email will be sent immediately and not go thorough messenger queue. This part needs to go back in the option explanations.

1. Perhaps flock() error should be logged using database. Meanwhile, the original code would also drop the queued data silently if email/jabber is not enabled.
2. When analyzing the original code, I found that emails are not immediately sent as described in the original explanation string. In fact, emails are queued, and the queue would only be processed when “cron.php” is run.

[Oleg]

Noone besides me cares?

1. Perhaps flock() error should be logged using database. Meanwhile, the original code would also drop the queued data silently if email/jabber is not enabled.

Well, it's one thing when an administrator disabled email and email is not sent (expected and correct behavior), and another thing entirely when email is enabled and not sent (misconfiguration, user error, code bug, etc.). There are multiple active topics on the main board when boards are not sending email as expected. Adding more silent failures is not going to improve the situation at all.

Further, you removed @ before flock calls. This means that if flock fails, which it could because it was disabled, or runs on file system that does not support locking (nfs), etc., users will get a nice dump of php complainage before the headers which could wreck whichever page they are on.

Locking is hard and doing it right requires knowing the theory and testing on real systems. You don't just remove parts of locking code that you did not write because you have no idea why they are there, and they could be very important in some environments. You also don't change locking code together with adding unrelated functionality in a single commit. First, such commits are nearly impossible to review. Second, if the changes turn out to break systems the locking changes cannot be reverted for debugging since they are intertwined with unrelated changes that would break functionality if touched.

In the ticket I linked to I supplied a locking implementation that retains as much of original code as possible, addresses a race condition, addresses other potential issues and is better factored. Do you have any objections to using it? Have you looked at it at all?

2. When analyzing the original code, I found that emails are not immediately sent as described in the original explanation string. In fact, emails are queued, and the queue would only be processed when “cron.php” is run.

If this is true it is a bug that should be fixed. Did you see branching in msg_email to use queue or send immediately?

[Oleg]

These concerns are less important than the ones in the previous post, maybe even nitpicky to some degree.

1. By investigating the cache/queue file, I found $package_size was originally stored as a string but not as an integer (which I expected).

Probably because config values are strings in the database.

Perhaps it is ok, but it may create other kinds of problems which are difficult to trace (in the future).

Do you have examples of such problems?

In fact, I think it is not easy to trace problems related to unexpected variable type when writing PHP program, so I always do type casting (for scalar types) in all variable assignment statements for my PHP programs.

I think you have taken this idea to cargo cult extremes. There cannot possibly be any use in typecasting in this expression:

Code: Select all

$now = (int) time();

Besides, you have to consider that the rest of the code does not do casts. If your code somehow relies on the fact that $package_size is an int, this ought to be documented. Otherwise, the casts are useless and are noise that one has to deal with when reading code.

4. The variable $process_history / $this->queue_data[$object]['process_history’] is just an array using time as key and $num_items as value which is very easy to find in the code. I think it is self-explanatory compared to $this->queue_data[$object]['data'].
5. $n is just a temporary variable, just like using $i in most for-loop. In fact, I had tried to think a better name, but it is difficult.

Maybe I have not stated this clearly enough, but what I meant was that I could not figure out what was going on from reading the code. So, for me #4 was not self-explanatory. I formulated a guess as to what was happening, but it would be much better if there was a comment with an explanation, which is what I requested (addition of explanation to the code).

Same with #5, if you can't come up with a concise variable name for $n write a comment explaining what it is. Really, just add the one line of comment explaining what it is.