messaginghub / pooled-jms

A JMS Connection pool for messaging applications supporting JMS 1.1 and 2.0 and Jakarta JMS clients
Apache License 2.0
49 stars 25 forks source link

is the expiry timeout implement? #20

Closed luisalves00 closed 3 years ago

luisalves00 commented 3 years ago

I've seen the following javadoc in the code:

 /**
     * Determines if this Connection has expired.
     * <p>
     * A ConnectionPool is considered expired when all references to it are released AND either
     * the configured idleTimeout has elapsed OR the configured expiryTimeout has elapsed.
     * Once a ConnectionPool is determined to have expired its underlying Connection is closed.
     *
     * @return true if this connection has expired.
     */
    public synchronized boolean expiredCheck() {

I don't see any way to configure an expiryTimeout. I need my connection to not live more than for example 15 min, is that possible?

tabish121 commented 3 years ago

You can call setConnectionIdleTimeout to control how long a connection sits idle in the pool before being closed.

luisalves00 commented 3 years ago

I don't mean idle. Even if the connection is in use, I want that when returning to the pool if total time of connection > X than it closes and creates a new one. But since it can have many sessions running, I'm not sure if it will solve anything. To give some context the need it's related with ARTEMIS-3102.

tabish121 commented 3 years ago

If the connection is in use the pool should not be messing with them, if that's what you are after then you probably shouldn't be using pooling as that basically defeats the point of connection pooling. Connections can idle timeout using the current pool configuration but the pool will not close connections that have been loaned out.

luisalves00 commented 3 years ago

My problem is that after receiving a javax.jms.JMSSecurityException, the connection will become unusable and all session using should stop using it and the connection should be dropped. This exception will happen after token used to do authentication is expired. I can control it to be long like 1 hour to 1 day, but I don't want it to last forever. Not using pooling will solve all my problems, but will hurt performance for sure.

tabish121 commented 3 years ago

First suggestion is to fix the root cause of the security exceptions. I'd argue that the JMS provider should be closing the connection if the connection wide security token has expired which should have triggered an onException call from the client that would notify the pool that the connection is now defunct.

Pooled-JMS version 1.2.0+ add additional liveness checking (as much as can be done with JMS) on return and borrow from the pool to try and detect unusable connections that have not thrown an error to the registered exception listener. When your application gets an JMSSecurityException from an API call it should be calling close on the connection as it should assume the connection is unusable which would return the connection to the pool and trigger liveness checks (1.2.0+).

luisalves00 commented 3 years ago

Hi,

Would you be open to add another catch before the "Exception ambiguous" for a specific RuntimeException that we could throw on our exception handler to flag that the connection should be thrown away?

                        // Sanity check the Connection and if it throws IllegalStateException we assume
                        // that it is closed or has failed due to some IO error.
                        try {
                            connection.getConnection().getExceptionListener();
                        } catch (IllegalStateException jmsISE) {
                            return false;
                        } catch (Exception ambiguous) {
                            // Unsure if connection is still valid so continue as if it still is.
                        }

Maybe I failed to understand something, but the connections seem to remain in the pool even after an exception.

tabish121 commented 3 years ago

The check validates as much as it can within the JMS specification limits. If IllegalStateException is thrown it will return false and the connection will be removed from the pool, otherwise we cannot make a judgement on the validity of the Connection as the JMS specification doesn't provide any clarity and so different clients could do different things there. A well behaved JMS client should have notified the pooled connection via on onException if the underlying IO layer has failed.

luisalves00 commented 3 years ago

I'm using activemq artemis client with the following code:

    void convertAndSend(String destinationName, Object payload, Map<String, Object> headers)
        throws JMSException {
        Connection connection = null;
        Session session = null;
        try {
            connection = connectionFactory.createConnection();
            connection.setExceptionListener(new CustomExceptionListener());

            session = connection.createSession(isSessionTransacted(), getSessionAcknowledgeMode());

            final ActiveMQDestination.TYPE type =
                ActiveMQDestination.TYPE.valueOf(config.getType().toString().toUpperCase());
            final Destination destination = ActiveMQDestination.createDestination(destinationName, type);
            MessageProducer messageProducer = session.createProducer(destination);

            //...

            messageProducer.send(message, getDeliveryMode(), getPriority(),
                getTimeToLive());

            if (isSessionTransacted()) {
                session.commit();
            }
        } catch (JsonProcessingException e) {
            throw new IllegalStateException("Failed to serialize the payload.", e);
        } finally {
            if (connection != null) {
                connection.close(); //release
                if (session != null) {
                    session.close();
                }
            }
        }
    }

    private static class CustomExceptionListener implements ExceptionListener {
        @Override
        public void onException(JMSException ex) {
            logger.error("Exception occurred need to trash the connection in the pool but can't throw a checked exception", ex);
        }
    }

maybe I'm doing something wrong, but I can't throw any checked exception o the Exception Listener, and therefore I don't know how can I tell the Pool that this connection should be trashed and not return to the pool.

tabish121 commented 3 years ago

The Pool intercepts the onException bits and will close the provider connection instance as soon as the onException is called, it forwards that error onto your listener later so you shouldn't need to do anything. Without knowing more I don't see anything more we could be doing here but I will look at the code again. What is the actual problem you are trying to solve?

luisalves00 commented 3 years ago

Think I've already mention, that my connection factory uses OAuth tokens instead of username+password. The problem is that tokens expire and cause some Security Exception, but the connection doesn't seem to be removed from the pool, as if a new connection was created a new token was generated and solve the problem. I'll debug a little bit more to try to understand what might be failing, as the following code should be closing the connection:

    @Override
    public void onException(JMSException exception) {
        // Closes the underlying connection and removes it from the pool
        close();

        if (parentExceptionListener != null) {
            parentExceptionListener.onException(exception);
        }
    }
tabish121 commented 3 years ago

I've added a small change to the handling and validation to v1.2.2 which I don't think helps in your case but you can try it out and see. Beyond that the best thing to do is create a minimal reproducer that can demonstrate the issue perhaps even a JUnit test for the existing Artemis client test suite and open a new issue.

tabish121 commented 3 years ago

Also as a side I wasn't aware that the Artemis client could support Oauth so it'd be interesting to know how that's working and what implications that has on the connection state when the Oauth token expires as I wouldn't be surprised that the client was doing the wrong thing.

luisalves00 commented 3 years ago

It doesn't support it. I've extended the ActiveMQXAConnectionFactory to pass a token in the password field. On the server I had to create a custom security manager that can verify the token. Everything works fine, but the server is not designed to drop the connection when the token expire. I've requested it on ARTEMIS-3102, but Justin told me to fix it on the client side.

tabish121 commented 3 years ago

Ok, so in this case there isn't much the pool can do. The connection needs to drop and signal an error via the exception listener and also throw the standard IllegalStateException errors from the connection APIs otherwise the pool just has no idea that it needs to evict the connection.

luisalves00 commented 3 years ago

Seems that it needs to be us to trigger the onException and I was expecting to be internally managed.

...
} catch (JMSException je) {
    if(connection != null) {
        connection.getExceptionListener().onException(je);
    }
    throw je;
} finally {
            if (connection != null) {
                connection.close(); //release
                if (session != null) {
                    session.close();
                }
            }
        }

with this strategy we will loose a message, every time token expires, that needs to be retried (re-active). Now I have a co-worker, extending Pool Factory to have the concept of max time to live (similar to HikariCP maxLifetime), where connection are thrashed if they are active for more time that the token lasts (minus some seconds). With this it will work smoothly (pro-active).

Thanks for the support.