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

JMS Session not closed if an exception is thrown #202

Closed
stuartpullinger opened this Issue May 17, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@stuartpullinger
Copy link
Contributor

stuartpullinger commented May 17, 2018

Description

There is a bug in the implementation of the Transmitter and NotificationTransmitter classes. If an exception is thrown whilst processing a JMS message, the Session object is not closed leading to all subsequent messages failing until the application is restarted. First reported by Jamie (@jsws).

@RKrahl: This also affects the IDS' Transmitter class.

Symptoms

In the server.log, we see many warnings like this:

[2018-05-03T13:22:25.696+0100] [Payara 4.1] [WARNING] [] [javax.jms.Connection.mqjmsra] [tid: _ThreadID=9857 _ThreadName=http-thread-pool::http-listener-2(52)] [timeMillis: 1525350145696] [levelValue: 900] [[
  MQJMSRA_DC4001: createSession():An active Session object already exists for this connection, only one active Session object allowed per connection.]]

In the icat.log, we see many errors like this:

2018-05-03 00:04:40,511 ERROR [http-thread-pool::http-listener-2(2)] Transmitter - Failed to send jms message createMany 130.246.39.170
2018-05-03 00:04:40,516 ERROR [http-thread-pool::http-listener-2(2)] ICAT - Unexpected failure in Java 1.8.0_151
javax.jms.JMSException: MQJMSRA_DC4001: createSession():An active Session object already exists for this connection, only one active Session object allowed per connection.
        at com.sun.messaging.jms.ra.DirectConnection.checkSessionsAllowed(DirectConnection.java:1064) ~[imqjmsra.jar:na]
        at com.sun.messaging.jms.ra.DirectConnection.createSession(DirectConnection.java:398) ~[imqjmsra.jar:na]
        at org.icatproject.core.manager.NotificationTransmitter.processMessage(NotificationTransmitter.java:70) ~[NotificationTransmitter.class:na]
...

Detail

The processMessage method on the Transmitter and NotificationTransmitter classes creates a Session object at the start and closes it at the end. If an exception is thrown in the method, it will never get to the close call and the Session object will remain. On the next call to the method, the Session creation will fail as there can only be one open Session at at time. The Java documentation states:
"Applications running in the Java EE web and EJB containers must not attempt to create more than one active (not closed) Session object per connection. If this method is called in a Java EE web or EJB container when an active Session object already exists for this connection then a JMSException may be thrown."

Solution

One solution may be to wrap the contents of the method in a try-finally clause and put the close call in the 'finally' block. However, elsewhere (such as the Apache ActiveMQ FAQ) it states that it is more efficient to create the objects at startup and reuse them.

Therefore I propose to refactor the method to move the creation of the objects (Session and MessageProducer) to the '@PostConstruct init' method and destruction to the '@PreDestroy exit' method.

@RKrahl

This comment has been minimized.

Copy link
Member

RKrahl commented May 18, 2018

I never had a look at the Transmitter class by now and I have only little knowledge about JMS. But your suggestion to move creation and destruction to @PostConstruct and @PreDestroy looks rather sensible.

@stuartpullinger stuartpullinger added this to the 4.9.2 milestone May 24, 2018

stuartpullinger added a commit that referenced this issue Nov 2, 2018

Merge pull request #203 from icatproject/iss202
Create and destroy JMS Session and Producer on object startup and destroy. Fixes #202

@stuartpullinger stuartpullinger removed the ready label Nov 2, 2018

@stuartpullinger

This comment has been minimized.

Copy link
Contributor

stuartpullinger commented Nov 19, 2018

Reopening this fix as the proposed fix fails the integration tests and was merged in error.

@stuartpullinger

This comment has been minimized.

Copy link
Contributor

stuartpullinger commented Nov 19, 2018

The original source for my change here stated that it was better to create the JMS Sessions and Producers ‘up front’ as it was more efficient. I have now found another source here which states that this doesn’t apply in JavaEE: the JMS API is the same in JavaSE and JavaEE but the behaviour is different. The JavaEE JMS API caches objects so repeated calls to create JMS Sessions and Producers are more efficient.

So repeated calls to create JMS Sessions is not a problem, but there is a second reason why it is preferable not to reuse the Session. The JMS Session object can only be accessed by one thread at a time. The Transmitter object which creates and keeps the session is a Singleton object but is accessed by multiple threads.

My solution is to reinstate Transmitter.java to how it was before the change above and move the creation of the Session to a try-with-resource statement – so the Session is recreated with each call to processMessage and will be closed properly if any exception is thrown (which will still fix the bug this was supposed to fix).

stuartpullinger added a commit that referenced this issue Jan 3, 2019

Merge pull request #210 from icatproject/threadsafe_jms_session
try-with-resource JMS Session to send messages. Fixes #202 - again!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment