git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: xiaoqiang zhao <zxq_yx_007@163.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Jan Viktorin <viktorin@rehivetech.com>,
	mst@kernel.org, pbonzini@redhat.com, mina86@mina86.com,
	Ramkumar Ramachandra <artagnon@gmail.com>
Subject: Re: [PATCH v5] send-email: --batch-size to work around some SMTP server limit
Date: Mon, 22 May 2017 11:26:41 +0200	[thread overview]
Message-ID: <CACBZZX5GYV50rjg9X602JHqFPaoofH9TwDf_-r_MDu8-rmNV6Q@mail.gmail.com> (raw)
In-Reply-To: <20170521125950.5524-1-zxq_yx_007@163.com>

On Sun, May 21, 2017 at 2:59 PM, xiaoqiang zhao <zxq_yx_007@163.com> 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?

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.

  parent reply	other threads:[~2017-05-22  9:27 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-21 12:59 [PATCH v5] send-email: --batch-size to work around some SMTP server limit xiaoqiang zhao
2017-05-22  2:14 ` Junio C Hamano
2017-05-22  3:19   ` Zhaoxiangqiang
2018-02-07 19:45   ` [PATCH] send-email: have default batch size when relogin delay is given Stefan Beller
2018-02-07 19:50     ` Eric Sunshine
2018-02-07 23:43       ` [PATCH] send-email: error out when relogin delay is missing Stefan Beller
2018-02-08  5:45         ` xiaoqiang zhao
2018-02-08  8:08         ` Eric Sunshine
2018-02-08 18:21           ` Stefan Beller
2018-02-08 22:00             ` Eric Sunshine
2018-02-12 19:44               ` Stefan Beller
2017-05-22  9:26 ` Ævar Arnfjörð Bjarmason [this message]
2017-05-23  7:46   ` [PATCH v5] send-email: --batch-size to work around some SMTP server limit Junio C Hamano
2017-05-23  8:30     ` Jan Viktorin
2017-05-23 12:55   ` 赵小强

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACBZZX5GYV50rjg9X602JHqFPaoofH9TwDf_-r_MDu8-rmNV6Q@mail.gmail.com \
    --to=avarab@gmail.com \
    --cc=artagnon@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mina86@mina86.com \
    --cc=mst@kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=viktorin@rehivetech.com \
    --cc=zxq_yx_007@163.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).