Test SMTP connection when claiming from pool

[swongu]

I'm using simple-java-mail 6.0.3 and wondering if there is a way to configure the mailer to check connectivity when claiming. I've found it's possible that broken connections in the pool are re-used when setting MailerGenericBuilder.withConnectionPoolExpireAfterMillis to a longer expiration time

Looking at the implementation I found that Allocator.allocateForReuse seems to be the appropriate place for this check. However the concrete TransportAllocator doesn't override this method -- would something like the following be reasonable?

@Overload
public Transport allocateForReuse(Transport transport) {
   ...
   try {
      if (transport.isConnected()) {
         transport.connect();
      }

      return transport;
   }
   ...
}

See https://github.com/eclipse-ee4j/mail/blob/1.6.3/mail/src/main/java/com/sun/mail/smtp/SMTPTransport.java#L1419 for SMTPTransport.isConnected

[bbottema]

Interesting addition, but I'm not sure the code would work there since Simple Java Mail marks a claimed Transport resource as faulty in case of any exception and so it would never allocate for reuse.

Unless that mechanism is broken...

[swongu]

In my testing after it’s claimed and an exception is encountered during use, the resource isn’t used again. So that’s working fine. The test I’m proposing is after claim and before usage Hopefully that makes sense. Really appreciated your quick reply!!

[bbottema]

Hmm, makes sense. I'd need to look into the performance overhead first though of a connection test.

If it is negligent, then no worries. Otherwise I'd like to introduce a cool down property so that it only rechecks the connection after a user defined threshold (so during bursts of emails it won't check the connection each reuse).

[fwiersENO]

The performance hit can be low when the NOOP command is used: it is just one extra command added to all the other commands required to send an email.
The benefit is that the command will fail when a connection was returned to the pool in a broken state (e.g. halfway a DATA sending operation).
The downside is that the NOOP command has issues (probably - I cannot verify this).
From the documentation at https://javaee.github.io/javamail/docs/api/com/sun/mail/smtp/package-summary.html :

mail.smtp.userset: If set to true, use the RSET command instead of the NOOP command in the isConnected method. In some cases sendmail will respond slowly after many NOOP commands; use of RSET avoids this sendmail issue. Defaults to false.

mail.smtp.noop.strict: If set to true (the default), insist on a 250 response code from the NOOP command to indicate success. The NOOP command is used by the isConnected method to determine if the connection is still alive. Some older servers return the wrong response code on success, some servers don't implement the NOOP command at all and so always return a failure code. Set this property to false to handle servers that are broken in this way. Normally, when a server times out a connection, it will send a 421 response code, which the client will see as the response to the next command it issues. Some servers send the wrong failure response code when timing out a connection. Do not set this property to false when dealing with servers that are broken in this way.

If you know the email server will respond properly to a NOOP command, a method like the one below can be used to validate a connection before "checkout of the pool":

    /**
     * Returns false if the transport connection is invalid and closed by this method,
     * returns true if the given transport connection is valid.
     */
    boolean validateTransportConnection(SMTPTransport transport) {

        boolean valid = false;
        try {
            /*
             * isConnected uses a "NOOP" command and expects a "250" return status.
             * If it fails, isConnected will close the connection.
             */
            valid = transport.isConnected();
            if (valid) {
                log.debug("Validated a SMTP transport connection for {}.", getDestinationName());
            } else {
                log.info("Closed an invalid SMTP transport connection for {}.", getDestinationName());
            }
        } catch (Exception e) {
            // Technically, a catch is not required since isConnected does not throw an exception.
            // But just to be on the safe side, catch any unexpected runtime error.
            log.info("SMTP transport connection no longer valid for {}: {}", getDestinationName(), e.toString());
            closeTransport(transport);
        }
        return valid;
    }

[bbottema]

It's a little confusing. Three things:

1.

The benefit is that the command will fail when a connection was returned to the pool in a broken state (e.g. halfway a DATA sending operation).

So this should not be possible. A transport that broke down would be marked faulty when returning to the pool, so it will be destroyed.

If I initially understood you correctly, your use case is about connections that were returned just fine (or simply kept warm but never used before) and the connection breaks down while idle in the connection pool. Then when claiming (allocating), the connection should be checked just to be sure.

2.

The downside is that the NOOP command has issues
(..)

I find this a little difficult to unify into a single simple api. I would like to solve this for my users on higher an abstraction level, but the whole 'if the server supports it' makes it confusing.

I would like to offer my users something like this:

mailerBuilder
   .withConnectionPoolDefensiveClaimingEnabled() // with mail.smtp.noop.strict
   .withConnectionPoolDefensiveClaimingLegacyMode() // turn off mail.smtp.noop.strict

Or something like that.

3.

Regarding mail.smtp.userset / mail.smtp.noop.strict, what would the solution be in pseudo code? I haven't had time to explore those properties yet, so I wonder what it would look like. Obviously I wouldn't want those properties to be active during actual connection test / email sending (the regular Mailer api), or otherwise I'm contaminating the Session object with properties meant for defensive claiming from the connection pool.

[fwiersENO]

Sorry for the confusion, I'll try to clarify.
1)
Yes, this is more about making sure an idle connection did not disconnect while being idle. But the added benefit is that if, for whatever reason, the connection was somehow returned to the pool in a bad state, the isConnected / NOOP command is likely to catch it.
2)
Just one "opt-in" option for the "check connection alive on pool checkout" function looks like a good start. This will then just use the defaults. If users find that the defauts do not work in some cases with some servers (e.g. you need the "LegacyMode" for some email-servers), that configuration option could be added later. I have a feeling that email-servers that cannot handle the defaults for the connection-check are in the minority and these email-servers could be maintained in a separate list that could be used to simply disable/skip the "check connection alive on pool checkout" function.
But as I said: only when there is a need for this "LegacyMode" function should it be considered. The worst that can happen when "isConnected" is used on check-out AND the email-server does not respond properly, is that a new connection is created and used. Overall, this may have so little performance impact that just using the defaults is good enough.
3)
Let's first see what the experiences are if the option "check connection alive on pool checkout" is turned on with just the defaults. Create a (unit) test that verifies that when the connection-check fails, a new connection is created and returned instead.

[bbottema]

It's been a while! Recently, in January, I've released a version that, when being reclaimed, reconnects if the connection was lost in the mean time. Would that solve this problem?

[bbottema]

Closing due to lack of response. Assuming the change covers the use case reported here.