Bug 33964 - Use Email::Sender::Transport::SMTP::Persistent for sending email
Summary: Use Email::Sender::Transport::SMTP::Persistent for sending email
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
: 30013 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-06-08 15:54 UTC by Kyle M Hall
Modified: 2023-08-18 13:10 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.03


Attachments
Bug 33964: Use Email::Sender::Transport::SMTP::Persistent for sending email (4.69 KB, patch)
2023-06-08 15:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33964: Use Email::Sender::Transport::SMTP::Persistent for sending email (4.73 KB, patch)
2023-07-06 15:34 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 33964: Use Email::Sender::Transport::SMTP::Persistent for sending email (4.83 KB, patch)
2023-07-20 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33964: (QA follow-up) Handle absence of smtp server (1.22 KB, patch)
2023-07-20 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33964: (QA follow-up) Syntax error in cpanfile (864 bytes, patch)
2023-07-25 01:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33964: (QA follow-up) Remove library from cpanfile (992 bytes, patch)
2023-07-25 11:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2023-06-08 15:54:56 UTC

    
Comment 1 Kyle M Hall 2023-06-08 15:57:29 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.
Comment 2 Kyle M Hall 2023-06-08 15:58:25 UTC
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.
Comment 3 David Cook 2023-06-09 05:19:19 UTC
Is this still in progress or should it be needs signoff?
Comment 4 Kyle M Hall 2023-06-09 10:15:07 UTC
(In reply to David Cook from comment #3)
> Is this still in progress or should it be needs signoff?

Yes please!
Comment 5 Eric Balluto 2023-06-30 12:35:59 UTC
Needs test plan
Comment 6 Kyle M Hall 2023-07-03 12:42:06 UTC
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!
Comment 7 David Cook 2023-07-05 02:26:26 UTC
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.
Comment 8 Sam Lau 2023-07-06 15:34:37 UTC
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>
Comment 9 Sam Lau 2023-07-06 15:37:03 UTC
(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! :)
Comment 10 David Cook 2023-07-10 23:15:00 UTC
(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.
Comment 11 Marcel de Rooy 2023-07-20 13:45:39 UTC
*** Bug 30013 has been marked as a duplicate of this bug. ***
Comment 12 Marcel de Rooy 2023-07-20 13:54:40 UTC
Just noting that libemail-sender-perl contains Email::Sender::Transport::SMTP::Persistent.

So thats fine already.
Comment 13 Marcel de Rooy 2023-07-20 14:31:13 UTC
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>
Comment 14 Marcel de Rooy 2023-07-20 14:31:16 UTC
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>
Comment 15 Marcel de Rooy 2023-07-20 14:34:51 UTC
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?
Comment 16 Marcel de Rooy 2023-07-20 14:38:51 UTC
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.
Comment 17 David Cook 2023-07-20 23:26:12 UTC
(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.
Comment 18 Marcel de Rooy 2023-07-21 06:34:26 UTC
(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.
Comment 19 David Cook 2023-07-21 07:00:30 UTC
(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.
Comment 20 Marcel de Rooy 2023-07-21 07:02:49 UTC
Module does this:

if (my $client = $self->_cached_client) {
    return $client if eval { $client->reset; $client->ok; };
Comment 21 David Cook 2023-07-21 07:05:41 UTC
Friday 5:05pm for me so I'll have to leave it with you folk for now. Happy Friday folks!
Comment 22 Marcel de Rooy 2023-07-21 07:07:13 UTC
RM: Reset is expected. So go ahead.
Comment 23 Tomás Cohen Arazi 2023-07-24 17:55:52 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 24 Tomás Cohen Arazi 2023-07-25 01:19:27 UTC
Created attachment 153857 [details] [review]
Bug 33964: (QA follow-up) Syntax error in cpanfile

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2023-07-25 11:26:17 UTC
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>
Comment 26 Fridolin Somers 2023-08-04 02:15:23 UTC
More enhancement no ?
Can this be backported ?
Comment 27 Kyle M Hall 2023-08-04 10:57:10 UTC
(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.
Comment 28 Fridolin Somers 2023-08-04 19:00:50 UTC
Pushed to 23.05.x for 23.05.03
Comment 29 Pedro Amorim 2023-08-18 13:10:15 UTC
Missing dependencies for 22.11.x. Not pushing.