Re:Re: [PATCH v5] send-email: --batch-size to work around some SMTP server limit

[赵小强]

At 2017-05-22 17:26:41, "Ævar Arnfjörð Bjarmason"  wrote:
>On Sun, May 21, 2017 at 2:59 PM, xiaoqiang zhao  wrote:
>> Some email servers (e.g. smtp.163.com) limit the number emails to be
>> sent per session(connection) and this will lead to a faliure when
>> sending many messages.
>
>This OK to me, the nits I had are addressed by Junio's reply.
>
>Looking at this the Nth time now though I wonder about this approach
>in general. In all your E-Mails I don't think you ever said /what/
>sort of error you had from the SMTP server, you just said you had a
>failure or an error, I assume you hit one of the die's in the
>send_message() function. Can you paste the actual error you get
>without this patch?
>

When I send a patch series which has 13 (plus cover) messages as a test, I got 
errors as follows and send-email quit when sending the 11th message:

MI:DMC 163 smtp14,EsCowAD3o71TKyRZTBlJHw--.20496S13 1495542613 
http://mail.163.com/help/help_spam_16.htm?ip=1.203.183.150=smtp14=1495542613

Follow the link above, I find two error code:

•450 MI:DMC 当前连接发送的邮件数量超出限制。请减少每次连接中投递的邮件数量
•451 MI:DMC 当前连接发送的邮件数量超出限制。请控制每次连接中投递的邮件数量

Translate  into English:
•450 MI:DMC The number of messages sent execeeds the limits. Please reduce the 
number of messages  to be sent  per connection.
•451 MI:DMC The number of messages sent execeeds the limits. Please control the 
number of messages to be sent per connection.

Although has different error code, but  says similar reason. Testing with 
--smtp-debug option produce the same error.

>I wonder if something like this would Just Work for this case without
>any configuration or command-line options, with the added benefit of
>just working for anyone with transitory SMTP issues as well (patch
>posted with -w, full version at
>https://github.com/avar/git/commit/acb60c4bde50bdcb62b71ed46f49617e2caef84e.patch):
>
>diff --git a/git-send-email.perl b/git-send-email.perl
>index 8a1ee0f0d4..c2d85236d1 100755
>--- a/git-send-email.perl
>+++ b/git-send-email.perl
>@@ -1363,6 +1363,10 @@ EOF
>die __("The required SMTP server is not
>properly defined.")
>}
>
>+   my $num_tries = 0;
>+   my $max_tries = 5;
>+   smtp_again:
>+   eval {
>if ($smtp_encryption eq 'ssl') {
>$smtp_server_port ||= 465; # ssmtp
>require Net::SMTP::SSL;
>@@ -1429,6 +1433,22 @@ EOF
>}
>$smtp->dataend() or die $smtp->message;
>$smtp->code =~ /250|200/ or die
>sprintf(__("Failed to send %s\n"), $subject).$smtp->message;
>+   1;
>+   } or do {
>+   my $error = $@ || "Zombie Error";
>+
>+   warn sprintf(__("Failed to send %s due to
>error: %s"), $subject, $error);
>+   if ($num_tries++ < $max_tries) {
>+   $smtp->quit if defined $smtp;
>+   $smtp = undef;
>+   $auth = undef;
>+   my $sleep = $num_tries * 3; # 3, 6, 9, ...
>+   warn sprintf(__("This is retry %d/%d.
>Sleeping %d before trying again"),
>+$num_tries, $max_tries, $sleep);
>+   sleep($sleep);
>+   goto smtp_again;
>+   }
>+   };
>}
>if ($quiet) {
>printf($dry_run ? __("Dry-Sent %s\n") : __("Sent
>%s\n"), $subject);
>
>Now that's very much a WIP and I don't have a server like that to test against.
>
>Having worked with SMTP a lot in a past life/job, I'd say it's *very*
>likely that you're just getting a /^4/ error code from 163.com,
>probably 421, which would make this logic even simpler. I.e. we could
>just adjust this to back-off for /^4/ instead of trying to handle
>arbitrary errors.
>
>Anyway, I'm not interested in pursuing that WIP patch, and I don't
>think perfect should be the enemy of the good here. Your patch works
>for you, doesn't really damage anything else, so if you're not
>interested in hacking up something like the above I think we should
>just take it.
>
>But I do think it would be very good to get a reply to you / details
>in the commit message about what error you get exactly in this
>scenario, see if you get better details with --smtp-debug, and if so
>paste that (sans any secret info like user/password you don't want to
>share).
>
>Then if we're poking at this code in the future we can maybe just fix
>this in some more general fashion while keeping this use-case in mind.

Re: [PATCH v4] send-email: --batch-size to work around some SMTP server limit

[赵小强]

> 在 2017年5月17日，07:49，Junio C Hamano  写道：
> 
> Junio C Hamano  writes:
> 
>> xiaoqiang zhao  writes:
>> ...
>>> Note:
>>>   Re-authentication will happen every $ messages, so it
>>> will be much more acceptable if you use some form of credential helper
>>> (e.g. the 'sendemail.smtppass' config option), otherwise you will have
>>> to retype password every time when asked.
>> 
>> I think this deserves to be in the end-user documentation (i.e. the
>> part of your patch that updates Documentation/git-send-email.txt).
> 
> Ah, this came out to be rather obliqu, but you do need an update to
> the documentation as part of this patch.
> 
> Thanks.

Sure！

Re: [PATCH v4] send-email: --batch-size to work around some SMTP server limit

[赵小强]

在 2017年5月17日，01:43，Ævar Arnfjörð Bjarmason  写道：

>>> Regards
>>> Jan
>> 
>> Thank you for reporting this，I will take a look .
> 
> You just need to initialize the variables you're using, see e.g. these
> existing ones:
> 
>my ($quiet, $dry_run) = (0, 0);
> 
> Just do the same for the ones you're adding.

Yes，has to be.

Re: [PATCH v4] send-email: --batch-size to work around some SMTP server limit

[赵小强]

> 在 2017年5月16日，20:10，Jan Viktorin  写道：
> 
> Hello,
> 
> with this patch applied to git 2.12, I could see:
> 
> Use of uninitialized value $batch_size in numeric eq (==) at 
> /usr/lib/git-core/git-send-email line 1679
> 
> when --batch-size is NOT used. See below...
> 
> On Sat, 13 May 2017 09:57:26 +0800
> xiaoqiang zhao  wrote:
> 
>> Some email servers (e.g. smtp.163.com) limit the number emails to be
>> sent per session(connection) and this will lead to a faliure when
>> sending many messages.
>> 
>> Teach send-email to disconnect after sending a number of messages
>> (configurable via the --batch-size= option), wait for a few
>> seconds (configurable via the --relogin-delay= option) and
>> reconnect, to work around such a limit.
>> 
>> Also add this two configure option for git config command.
>> 
>> Note:
>>   Re-authentication will happen every $ messages, so it
>> will be much more acceptable if you use some form of credential helper
>> (e.g. the 'sendemail.smtppass' config option), otherwise you will have
>> to retype password every time when asked.
>> 
>> Signed-off-by: xiaoqiang zhao 
>> ---
>> contrib/completion/git-completion.bash |  2 ++
>> git-send-email.perl| 18 ++
>> 2 files changed, 20 insertions(+)
>> 
>> diff --git a/contrib/completion/git-completion.bash 
>> b/contrib/completion/git-completion.bash
>> index af658995d..29496353a 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -2608,6 +2608,8 @@ _git_config ()
>>sendemail.thread
>>sendemail.to
>>sendemail.validate
>> +sendemail.smtpbatchsize
>> +sendemail.smtprelogindelay
>>showbranch.default
>>status.relativePaths
>>status.showUntrackedFiles
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index eea0a517f..071d1ab9d 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -81,6 +81,10 @@ git send-email --dump-aliases
>>  This setting forces to use one of the 
>> listed mechanisms.
>> --smtp-debug<0|1>  * Disable, enable Net::SMTP debug.
>> 
>> +--batch-size  * send max  message per connection.
>> +--relogin-delay   * delay  seconds between two 
>> successive login, default to 1,
>> + This option can only be used with 
>> --batch-size
>> +
>>   Automating:
>> --identity* Use the sendemail. options.
>> --to-cmd  * Email To: via ` \$patch_path`
>> @@ -153,6 +157,7 @@ my $have_email_valid = eval { require Email::Valid; 1 };
>> my $have_mail_address = eval { require Mail::Address; 1 };
>> my $smtp;
>> my $auth;
>> +my $num_sent = 0;
>> 
>> # Regexes for RFC 2047 productions.
>> my $re_token = qr/[^][()<>@,;:\\"\/?.= \000-\037\177-\377]+/;
>> @@ -216,6 +221,7 @@ my ($cover_cc, $cover_to);
>> my ($to_cmd, $cc_cmd);
>> my ($smtp_server, $smtp_server_port, @smtp_server_options);
>> my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
>> +my ($batch_size, $relogin_delay);
>> my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);
>> my ($validate, $confirm);
>> my (@suppress_cc);
>> @@ -247,6 +253,8 @@ my %config_settings = (
>> "smtppass" => \$smtp_authpass,
>> "smtpdomain" => \$smtp_domain,
>> "smtpauth" => \$smtp_auth,
>> +"smtpbatchsize" => \$batch_size,
>> +"smtprelogindelay" => \$relogin_delay,
>> "to" => \@initial_to,
>> "tocmd" => \$to_cmd,
>> "cc" => \@initial_cc,
>> @@ -358,6 +366,8 @@ $rc = GetOptions(
>>"force" => \$force,
>>"xmailer!" => \$use_xmailer,
>>"no-xmailer" => sub {$use_xmailer = 0},
>> +"batch-size=i" => \$batch_size,
>> +"relogin-delay=i" => \$relogin_delay,
>> );
>> 
>> usage() if $help;
>> @@ -1664,6 +1674,14 @@ foreach my $t (@files) {
>>}
>>}
>>$message_id = undef;
>> +$num_sent++;
>> +if ($num_sent == $batch_size) {
> 
> This is the line. I think, batch_size can be sometimes uninitialized
> while this statement is executed.
> 
> Regards
> Jan

Thank you for reporting this，I will take a look .
> 
>> +$num_sent = 0;
>> +$smtp->quit if defined $smtp;
>> +undef $smtp;
>> +undef $auth;
>> +sleep($relogin_delay);
>> +}
>> }
>> 
>> # Execute a command (e.g. $to_cmd) to get a list of email addresses

Re: [PATCH v3] send-email: --batch-size to work around some SMTP server limit

[赵小强]

> 在 2017年5月8日，12:11，Junio C Hamano  写道：
> 
> Two suggestions.
> 
> (1) I do not think $smtp is always valid when we come here; it is
> unsafe to unconditionally say $smtp->quit like this patch does.
> 
>$smtp->quit if defined $smtp;
> 
> may help codepaths like $dry_run and also the case where
> $smtp_server is the absolute path to a local program.
> 

Hmm，missed this code path.

> (2) You are setting $auth to zero to force re-authentication to
> happen.  It would be more consistent to the state of $auth that
> is not-yet-used to "undef $auth;" here instead.  After all, the
> variable starts its life in an undefined state.
> 
> 
> So all in all
> 
>$smtp->quit if defined $smtp;
>undef $smtp;
>undef $auth;
> 
> perhaps?
> 
> This change of course forces re-authentication every N messages,
> which may not hurt those who use some form of credential helper, but
> that may be something we want to mention in the log message.

Yes, it' s better to undef $auth here. I will update the commit message next 
version.

Thank you very much for your helpful suggestions ！

> 
>> +sleep($relogin_delay);
>> +}
>> }
> 
> Thanks.

Re: [PATCH v2] send-email: new options to walkaround email server limits

[赵小强]

> 在 2017年5月2日，10:24，Junio C Hamano  写道：
> 
> But I am having a huge problem seeing how this patch is correct.  It
> always is troubling to see a patch that makes the behaviour of a
> program change even when the optional feature it implements is not
> being used at all.  Why does it even have to touch smtp_auth_maybe?
> Why does the updated smtp_auth_maybe have to do quite different
> things even when batch-size is not defined from the original?
> What is that new "Auth use saved password. \n" message about?
You are right！I need to correct this.
> After reading the problem description in the proposed log message,
> the most natural update to achieve the stated goal is to add code to
> the loop that has the only caller to send_message() function, I
> would think.  The loop goes over the input files and prepares the
> variables used in send_message() and have the function send a single
> message, initializing $smtp as necessary but otherwise reusing $smtp
> the previous round has prepared.  So just after $message_id is
> undefed in the loop, I expected that you would count "number of
> messages sent so far during this session", and when that number
> exceeds the batch size, disconnect $smtp and unset the variable,
> and sleep for a bit, without having to change anything else.

Agree！

Re: [PATCH] send-email: new option to walkaround email server limits

[赵小强]

在 2017年5月2日，17:32，Paolo Bonzini  写道：

>> On 29/04/2017 14:26, xiaoqiang zhao wrote:
>> Some email server(e.g. smtp.163.com) limits a fixed number emails to be send 
>> per
>> session(connection) and this will lead to a send faliure.
>> With --split  option, a auto reconnection will occur when number of 
>> sended
>> email reaches  and the problem is solved.
>> 
>> Signed-off-by: xiaoqiang zhao 
> 
> I think you should also add a matching configuration option, or you are
> going to forget it on the command line sooner or later!
> 
> Paolo

Good idea！

Re: [PATCH] send-email: new option to walkaround email server limits

[赵小强]

Thanks for your reply , Junio !

> 在 2017年5月1日，09:54，Junio C Hamano  写道：
> 
> here.  We need to find a better name for the option.  Perhaps
>   "--batch-size=", "--max-messages-per-connection=" or
>   something?
> 

--batch-size is ok with me

> - The code seems to do the "logging out and the logging back in"
>   dance without any delay in between.  Is it something we want to
>   consider to add a small (possibly configurable) delay in between?

It' s a good idea to have a configurable delay.