Description
Kyle M Hall (khall)
2023-06-08 15:54:56 UTC
As described in bug 30013, some outgoing SMTP services ( such as Gmail ) do not like Koha's current behavior of initiating a new connection for each email sent. If we switch from Email::Sender::Transport::SMTP to Email::Sender::Transport::SMTP::Persistent and store the object for the duration of the message queue processing, this should solve that issue. Created attachment 152211 [details] [review] Bug 33964: Use Email::Sender::Transport::SMTP::Persistent for sending email As described in bug 30013, some outgoing SMTP services ( such as Gmail ) do not like Koha's current behavior of initiating a new connection for each email sent. If we switch from Email::Sender::Transport::SMTP to Email::Sender::Transport::SMTP::Persistent and store the object for the duration of the message queue processing, this should solve that issue. Is this still in progress or should it be needs signoff? (In reply to David Cook from comment #3) > Is this still in progress or should it be needs signoff? Yes please! Needs test plan Test Plan: 1) Verify your server can successfully send email via SMTP 2) Install Email::Sender::Transport::SMTP::Persistent ( sudo cpanm Email::Sender::Transport::SMTP::Persistent ) 3) Apply this patch 4) Restart all the things! 5) Enqueue some email message 6) Run process_message_queue.pl 7) Verify the emails were sent! I'm keen to QA this one, so I'm just waiting for someone else to sign it off. We've been setting up a lot more SMTP lately. I'm curious to compare the speed of Email::Sender::Transport::SMTP vs Email::Sender::Transport::SMTP::Persistent as well. I suspect that the latter will be much nicer for performance. Created attachment 153101 [details] [review] Bug 33964: Use Email::Sender::Transport::SMTP::Persistent for sending email As described in bug 30013, some outgoing SMTP services ( such as Gmail ) do not like Koha's current behavior of initiating a new connection for each email sent. If we switch from Email::Sender::Transport::SMTP to Email::Sender::Transport::SMTP::Persistent and store the object for the duration of the message queue processing, this should solve that issue. Signed-off-by: Sam Lau <samalau@gmail.com> (In reply to David Cook from comment #7) > I'm keen to QA this one, so I'm just waiting for someone else to sign it off. > All yours! :) (In reply to Sam Lau from comment #9) > (In reply to David Cook from comment #7) > > I'm keen to QA this one, so I'm just waiting for someone else to sign it off. > > > All yours! :) Thanks, Sam. I probably won't get to this until next week, so happy for someone else to swoop in, but failing that I'll take a look then. *** Bug 30013 has been marked as a duplicate of this bug. *** Just noting that libemail-sender-perl contains Email::Sender::Transport::SMTP::Persistent. So thats fine already. Created attachment 153723 [details] [review] Bug 33964: Use Email::Sender::Transport::SMTP::Persistent for sending email As described in bug 30013, some outgoing SMTP services ( such as Gmail ) do not like Koha's current behavior of initiating a new connection for each email sent. If we switch from Email::Sender::Transport::SMTP to Email::Sender::Transport::SMTP::Persistent and store the object for the duration of the message queue processing, this should solve that issue. Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153724 [details] [review] Bug 33964: (QA follow-up) Handle absence of smtp server Resolve: Use of uninitialized value in hash element at /usr/share/koha/C4/Letters.pm line 1472. Use of uninitialized value in hash element at /usr/share/koha/C4/Letters.pm line 1473. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I was still looking for some kind of confirmation that it really works as advertised using one connection. Cannot easily read it from the logs: Here a short impression for two notices sent in one run: WITH PATCHES 2023-07-20 14:13:06 1qMUOv-0013tQ-UW <= noreply@DOMAIN H=(localhost.localdomain) [127.0.0.1] P=esmtp S=647 id=16898623860.4c95DE.253291@master 2023-07-20 14:13:06 1qMUOv-0013tQ-Vl <= noreply@DOMAIN H=(localhost.localdomain) [127.0.0.1] P=esmtp S=621 id=16898623861.2FAcE0.253291@master WITHOUT PATCHES 2023-07-20 14:14:43 1qMUQV-0013w2-Lp <= noreply@DOMAIN H=(localhost.localdomain) [127.0.0.1] P=esmtp S=650 id=16898624830.dE16C1Fba.253453@master 2023-07-20 14:14:44 1qMUQV-0013w5-N3 <= noreply@DOMAIN H=(localhost.localdomain) [127.0.0.1] P=esmtp S=624 id=16898624831.B6fAaA9BB.253453@master I only see that in the part with the patches the identifiers are closer to each other: both start with 1qMUOv-0013tQ While without patches the second part is already different. 1qMUQV-0013w2-Lp 1qMUQV-0013w5-N3 Not sure what this proves? Exim told me this: When two or more messages are delivered down a single SMTP connection, an asterisk follows the remote IP address (and port if enabled) in the log lines for the second and subsequent messages. (In reply to Marcel de Rooy from comment #15) > I was still looking for some kind of confirmation that it really works as > advertised using one connection. You can use tcpdump to watch the SMTP port. I've set messaging preferences for check in and check out and generated 2 pending emails. I then manually run ./misc/cronjobs/process_message_queue.pl Without the patch, I'm seeing 2 different TCP connections (one for each email). With the patch, I'm seeing only 1 TCP connection and both emails go out. Looks good to me. Looking forward to this one going in. (In reply to David Cook from comment #17) > (In reply to Marcel de Rooy from comment #15) > You can use tcpdump to watch the SMTP port. I was expecting an answer like that somehow :) When I look at the dump, I see SMTP traffic on the same port at the sending side while seeing two different ports without these patches. And I see Reset with the patches, not without. (In reply to Marcel de Rooy from comment #18) > (In reply to David Cook from comment #17) > > (In reply to Marcel de Rooy from comment #15) > > You can use tcpdump to watch the SMTP port. > > I was expecting an answer like that somehow :) Hehe. I'm nothing if not predictable. > When I look at the dump, I see SMTP traffic on the same port at the sending > side while seeing two different ports without these patches. And I see Reset > with the patches, not without. I did notice the resets but neglected to look into them further. It does suggest a TCP connection hangup rather than a graceful close. Maybe there's a follow-up necessary to gracefully close the persistent connections at the end of the calling function. Module does this: if (my $client = $self->_cached_client) { return $client if eval { $client->reset; $client->ok; }; Friday 5:05pm for me so I'll have to leave it with you folk for now. Happy Friday folks! RM: Reset is expected. So go ahead. Pushed to master for 23.11. Nice work everyone, thanks! Created attachment 153857 [details] [review] Bug 33964: (QA follow-up) Syntax error in cpanfile Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 153869 [details] [review] Bug 33964: (QA follow-up) Remove library from cpanfile Email::Sender::Transport::SMTP::Persistent is part of the Email::Sender distribution, and a git diff on the repository doesn't show any difference. The patch author just took the number from MetaCPAN. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> More enhancement no ? Can this be backported ? (In reply to Fridolin Somers from comment #26) > More enhancement no ? > Can this be backported ? This fixes brokenness with using Gmail as an SMTP server for Koha, so I would consider it a bug fix. Pushed to 23.05.x for 23.05.03 Missing dependencies for 22.11.x. Not pushing. |