SingleConnectionFactory stops connection on every stop/close on connection proxy [SPR-14499]

[spring-issuemaster]

Piotr Klimczak opened SPR-14499 and commented

1st of all thanks for really great job, the Spring community is doing.

Now, we just started migrating from spring 3.2.X to 4.3.1 and we found that our JMS connectivity floods logs with warnings:
Setup of JMS message listener invoker failed for destination 'X.Y.Z' - trying to recover. Cause: Connection closed

The problem was introduced by this issue: #15030 and this commit: 4927c90.
(by adding localStart, localStop methods)

The purpose of SingleConnectionFactory suppose to be sharing same connection between threads. How sharing the same connection suppose to work at all, if it can be closed any time by any thread?
As we are finding this incorrect, that is the reason why this issue was created.

In fact, current implementation makes SingleConnectionFactory completely unusable.
While it can pass unit tests in simple scenarios (it passes our tests too), it definitely fails in runtime, where we are using about 100 consumers to different destinations.

---

Affects: 4.1.9, 4.2.7, 4.3.1

[spring-issuemaster]

Juergen Hoeller commented

Which code path leads to the actual closing of the underlying connection in your scenario? We mean to track local start/stop state but only really stop the underlying connection if all clients are in stop state. And we don't mean to close the underlying connection at all before the SingleConnectionFactory itself shuts down.

[spring-issuemaster]

Piotr Klimczak commented

Hi Juergen!

Huge thanks for quick answer.
I believe those 2 methods are causing the problem:

		private void localStart() throws JMSException {
			synchronized (connectionMonitor) {
				if (!this.locallyStarted) {
					this.locallyStarted = true;
					if (startedCount == 0 && connection != null) {
						connection.start();
					}
					startedCount++;
				}
			}
		}

		private void localStop() throws JMSException {
			synchronized (connectionMonitor) {
				if (this.locallyStarted) {
					this.locallyStarted = false;
					if (startedCount == 1 && connection != null) {
						connection.stop();
					}
					if (startedCount > 0) {
						startedCount--;
					}
				}
			}
		}

I can see then intention in the code, which you have mentioned in your previous comment: to close only if all "references" or "start" were followed by "stops".
But with current implementation, this will not work as startedCount will always be maximum 1.
This is because "startedCount + +" only occurs, when "locallyStarted" == false, but "locallyStarted" is changed to true after 1st start- thus "startedCount + +" will not occur for any other start.
This might be the real cause of the problem.

If that is true, then solution will be as simple as:

		private void localStart() throws JMSException {
			synchronized (connectionMonitor) {
				if (!this.locallyStarted) {
					this.locallyStarted = true;
					if (startedCount == 0 && connection != null) {
						connection.start();
					}
				}
				startedCount++; // <---------------  MOVED HERE
			}
		}

		private void localStop() throws JMSException {
			synchronized (connectionMonitor) {
				if (this.locallyStarted) {
					this.locallyStarted = false;
					if (startedCount == 1 && connection != null) {
						connection.stop();
					}
				}
				if (startedCount > 0) { // <--------------- ALSO MOVED THIS HERE
					startedCount--;   // <--------------- ALSO MOVED THIS HERE
				}                                  // <--------------- ALSO MOVED THIS HERE
			}
		}

Hope that helps.

Regards
Piotr Klimczak

[spring-issuemaster]

Piotr Klimczak commented

I was provided with some time from my manager as it is important for him to get this solved before our next release, so will try get some more analysis.
But so far i can see that createConnection() implementation change is also a problem as it now returns new proxy every time, while in past it was returning same proxy.
So solution suggested in my previous comment will not work. Basically problem is more complex.
Will try to come with some more thoughts soon.

Cheers

[spring-issuemaster]

Juergen Hoeller commented

I'll be spending some time on this ASAP as well since our 4.3.2 release is scheduled for Wednesday next week (July 27th).

As for the connection closing that you're experiencing, I only really see that happening on actual SingleConnectionFactory shutdown... or recovery in case of an ExceptionListener-propagated exception. The localStart / localStop code would rather just influence connection start/stop state, or am I missing something? Are you possibly getting connection close exceptions once the connection has been stopped (but not technically closed)?

[spring-issuemaster]

Piotr Klimczak commented

Indeed, sorry for misunderstanding.
The problem is stopping, not closing.
And it is ActiveMQ which doesn't want to start connection which was previously stopped.

Please put this issue on hold for now, as in fact it might be ActiveMQ issue which is refusing to start stopped connection.

Still doing analysis.
Yesterday haven't got enough time for deeper understanding, doing that now as got agreement from my manager.

[spring-issuemaster]

Piotr Klimczak commented

So finally found that it is ActiveMQ issue with they PooledConnection implementation, which doesn't allow to start a stopped connection.
There is no problem in spring, connection stops are happening properly as crafted some unit tests in spring to repeat our scenario and spring behaves properly.

Sorry for confusion and thanks a lot for your time.

[spring-issuemaster]

Juergen Hoeller commented

Alright, no worries.

As for using Spring's SingleConnectionFactory with ActiveMQ, there's no real need for combining it with a PooledConnectionFactory there... You could just as well point SingleConnectionFactory directly to the native ActiveMQConnectionFactory instead, since it is going to hold on to its connection for the lifetime of the application anyway, even if some other components happen to use a PooledConnectionFactory bound to the same native factory.