Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: auto-reconnect (if needed) when reclaiming a Transport connection from the SMTP connection pool (to avoid needless errors) #430

Closed
lopardo opened this issue Dec 27, 2022 · 6 comments

Comments

@lopardo
Copy link

lopardo commented Dec 27, 2022

Hi, first I'd like to thank the authors of this library, it's just what I needed and the API is awesome.

I have an email notification service that uses 2 different SMTP servers depending on some rules (so they're independent, I have 2 separate Mailer instances).

They sometimes need to send a bunch of emails at the same time, so I'd like to reuse the connections as much as possible to improve performance.

After testing my Mailers with a 60 second expiration time for the connection pool like this:

MailerBuilder.withSMTPServer(host, port, username, password)
    .withConnectionPoolExpireAfterMillis(60000)

One of the SMTP servers is OK with that, but the other seems to allow about 30 seconds before disconnecting the client. I know most servers only allow a few seconds before dropping the connection, but I'd like to take advantage of the first server.

This is the exception I get when the connection times out:

org.simplejavamail.mailer.internal.MailerException: Failed to send email [<211055192.7.1672161843409@host.lan>], reason: Third party error
	at org.simplejavamail.mailer.internal.SendMailClosure.handleException(SendMailClosure.java:97)
	at org.simplejavamail.mailer.internal.SendMailClosure.executeClosure(SendMailClosure.java:89)
	at org.simplejavamail.mailer.internal.AbstractProxyServerSyncingClosure.run(AbstractProxyServerSyncingClosure.java:56)
	at org.simplejavamail.mailer.internal.MailerImpl.sendMail(MailerImpl.java:345)
	at org.simplejavamail.mailer.internal.MailerImpl.sendMail(MailerImpl.java:331)

Caused by: com.sun.mail.smtp.SMTPSendFailedException: 421 Timeout - closing connection
	at com.sun.mail.smtp.SMTPTransport.issueSendCommand(SMTPTransport.java:2374)
	at com.sun.mail.smtp.SMTPTransport.mailFrom(SMTPTransport.java:1808)
	at com.sun.mail.smtp.SMTPTransport.sendMessage(SMTPTransport.java:1285)
	at org.simplejavamail.mailer.internal.util.TransportRunner.lambda$sendMessage$0(TransportRunner.java:48)
	at org.simplejavamail.mailer.internal.util.TransportRunner.sendUsingConnectionPool(TransportRunner.java:78)
	at org.simplejavamail.mailer.internal.util.TransportRunner.runOnSessionTransport(TransportRunner.java:64)
	at org.simplejavamail.mailer.internal.util.TransportRunner.sendMessage(TransportRunner.java:47)
	at org.simplejavamail.mailer.internal.SendMailClosure.executeClosure(SendMailClosure.java:84)

I guess I could check the cause and retry myself, but maybe a setting could be added to automatically check if the connection is still valid before attempting to send an email?

Like adding at TransportRunner.java:48 :

if (!transport.isConnected()) {
    transport.connect();
}

My previous quick and dirty implementation using JavaMail 1.6 directly reused a single Session/SmtpTransport instance (pretty much like this example: https://stackoverflow.com/a/30932969), checking transport.isConnected() each time before attempting to send an email and that seemed to work OK (it only reconnected as necessary), but obviously it was limited to a single thread.

Thanks!

@bbottema
Copy link
Owner

bbottema commented Jan 5, 2023

Hhm, interesting idea. The challenge for me is the fact that the location where the Transport is used is different from the location where the Transport is initialized. In other words, by the time you would want to check if a connection is still there and potentially reconnect, the knowledge is to do so is gone and no link back to the factory to reinitialize a transport.

I would have to rework the Batch Module significantly to somehow encapsulate the initialization logic so it can be triggered again when the connection is required. It would significantly reduce the risk timeout errors significantly though.

@bbottema
Copy link
Owner

bbottema commented Jan 5, 2023

Hhm, maybe not. I already built in an allocateForReuse hook into the object pool, which is called on Transport instances being reclaimed just before being used...

bbottema added a commit to simple-java-mail/smtp-connection-pool that referenced this issue Jan 5, 2023
…ances when being claimed for imminent reuse.
@bbottema
Copy link
Owner

bbottema commented Jan 5, 2023

(..)
Like adding at TransportRunner.java:48 :

if (!transport.isConnected()) {
    transport.connect();
}

This is not a good idea, because this is the code used when not using the Batch Module. In this scenario it is assumed any mailing actions finish and close automatically so the JVM won't be blocked. So reusing a Transport instance here and reconnect is against this idiom.

However, I fixed it in the SMTP Connection Pool sub project instead. Before I update Simple Java Mail and release a new version, could you manually update your smtp-connection-pool dependency to version 2.1.2 and verify it works for you?

<dependencyManagement>
    <dependency>
        <groupId>org.simplejavamail</groupId>
        <artifactId>smtp-connection-pool</artifactId>
        <version>2.1.2</version>
    </dependency>
</dependencyManagement>

@bbottema bbottema added this to the 7.6.1 milestone Jan 5, 2023
@bbottema bbottema changed the title Add setting to check connection before sending? Enhancement: auto-reconnect (if needed) when reclaiming a Transport connection from the SMTP connection pool (to avoid needless errors) Jan 16, 2023
@bbottema
Copy link
Owner

I had to update the main project to use a newer version of smtp-connection-pool anyway, so now this change is hiking along for the ride. Enjoy it in 7.7.0!

@lopardo
Copy link
Author

lopardo commented Jan 17, 2023 via email

@bbottema
Copy link
Owner

Did you get a chance to have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants