Server side thread leak?

[galder.zamarreno]

Re: https://jira.jboss.org/jira/browse/JBAS-6665

Ron/Richard, is this happening in clustered or non clustered tests? Or both?

My thoughts:

Unified invokers (read EJB2) do not work like the the EJB3 ones. The unified one caches client in the very 1st invocation and uses that all the time (versus create/connect/disconnect for every invocation). In the case of clustered unified invoker, every time a new locator is used, a new client is created and used. While the same locator is used invocation after invocation, the same client is used. When the locator changes, a new client is created and connected to.

Not sure whether EJB2s should work exactly the same as EJB3s. In the case of non-clustered unified invoker, caching and reusing the same client all the time would probably yield better performance. When would we call disconnect in this case? Dunno right now.

In the case of clustered invokers, I think disconnecting the current client before creating/connecting a new one would make sense as otherwise you're leaking clients (this probably needs a jira, FAO Brian thoughts?)

Finally, why is the default value not to run the idle timeout task? For EJB2 non clustered beans, with current design, this idle timeout functionality should be enabled if we wanna leave it as it is. Actually, probably with clustered as well, (i.e. SFSBs that have first available lbp).

[jesper.pedersen]

Richard, the threads that identifies them self as "WorkManager" are from the JCA WorkManager thread pool.

See

deploy/jca-jboss-beans.xml

The thread pool is backed by the implementation in the util project:

org.jboss.util.threadpool.BasicThreadPool

See http://www.jboss.org/community/docs/DOC-9324 for configuration options.

The job of each thread is to execute a JCA Work instance - and then return to the pool.

[rachmato]

Hi Galder

The creeping thread count increase has only been observed so far when running the ant target jboss-all-config-tests, which runs a lot of the basic AS tests against a single instance of the 'all' configuration.

The little report generator I wrote isn't applicable to a two server configuration, but could be adapted.

[rachmato]

Notwithstanding Galder's comments, it seems as though the bulk of what is being seen may be a non-problem and just a matter of having large pool sizes for threads with thread reaping turned off or with long delay.

I'll try a run using smaller pool sizes and keep alive times for both JCA and Remoting
and see what happens.

Thanks for your help...

[brian.stansberry]

In the case of clustered invokers, I think disconnecting the current client before creating/connecting a new one would make sense as otherwise you're leaking clients (this probably needs a jira, FAO Brian thoughts?)

There are really 2 cases here:

a) switching to a new Client due to failover. In that case the InvokerLocator is removed from the set of targets and disconnecting seems like an obvious right thing to do.

b) switching to a new Client due to normal load balancing (e.g. RoundRobin) with an expectation that we may end up creating another Client later for the same InvokerLocator. That, if I understand the code correctly, will end up using the already-existing ClientInvoker

The b) case is more tricky. If in the non-clustered case we end up deciding to call disconnect after every invocation, then the clustered case should as well. If non-clustered doesn't disconnect on every invocation, we can revisit. Sounds like disconnecting is necessary though, with the "invokerDestructionDelay" as the workaround to cut down on the waste associated with that.

[brian.stansberry]

Shouldn't the AS side be setting the "idleTimeout" config though? Seems like an appserver shouldn't be counting on the client to properly manage its threads.

[rachmato]

I've done another run of the testsuite with smaller pool sizes JRA and Remoting and thread reaping turned on for Remoting. I've attached some comments and a new report file to the original JIRA issue (sorry for the back and forth).

The reduced pool sizes did not affect the passing/failing of tests as far as I can tell, and yet reduced the threading resources considerably - active thread count down by about 1/3. I've gone through and added the (+) as before and confirmed that WorkerThreads and WorkManager threads are being reclaimed.

This sort of change alleviates the (original) problem I was having running the testsuite on HPUX where I was getting OOME: cannot allocate native thread due to no memory left for allocating thread stacks. In that case, the problem was also related to a very high -Xmx setting which left less memory available outside the JVM for allocating thread stacks. Lowering the -Xmx value on the 'all' configuration from 512m to 384m (which is closer to the AS 4.2/4.3 value of 256m) allowed more threads to be created and virtually eliminated the problem.

[galder.zamarreno]

Sounds like disconnecting is necessary though, with the "invokerDestructionDelay" as the workaround to cut down on the waste associated with that.

I agree. It looks to me the right thing to do is to connect()/disconnect(). The only reason I could see to leave it as it is would be if creation/connect were noticeable more expensive operations compared to caching it and reusing the same client all the time. I don't see it being much more expensive although the ClientInvoker location done in 2.2.x happens within a big synchronized block.

A jira on this could verify the performance of different options.

[ron_sigal]

It's good to hear from Richard that turning on idle thread reaping is beneficial on the server side, but there's still the issue of leakage on the client side. Even if a worker thread is harvested, which results in closing its socket, the associated socket on the client side would remain in the connection pool until an invoker tries to use it.

The only reason I could see to leave it as it is would be if creation/connect were noticeable more expensive operations compared to caching it and reusing the same client all the time. I don't see it being much more expensive although the ClientInvoker location done in 2.2.x happens within a big synchronized block.

If the ClientInvoker already exists, control pops out of the synchronized block near the beginning.

I agree that turning on idle thread reaping is a good thing, and I agree that avoiding client side leakage is a good thing. The most straightforward way of doing the latter is to use the "invokerDestructionDelay" facility. Here are two related threads: "Remoting issue" in the "Design of EJB 3.0" forum

http://www.jboss.org/index.html?module=bb&op=viewtopic&t=130264

and "JBREM-877: New Socket Connection is being Created for Every" in the Remoting Users forum

http://www.jboss.org/index.html?module=bb&op=viewtopic&t=126382&postdays=0&postorder=asc&start=0.

Note the following quote from the latter thread:

"kiwi_clive" wrote:

Hi Ron,

Many thanks for your help. We saw an immediate performance gain, halving response time. This has proved to be the single most effective performance enhancement in our tuning efforts so many thanks for providing the fix.

Most appreciated !

Clive

I can think of an alternative: a TimerTask that reaps unusable sockets. But given the preceding quote I'm not sure if it's necessary.

-Ron

[brian.stansberry]

Sounds then like there are two things that need to be done:

1) Fix UnifiedInvokerProxy/UnifiedInvokeHAProxy to call disconnect.

2) Decide if we want to add the "invokerDestructionDelay" as a default config on the AS's Remoting Connectors.

I guess an issue with #2 is it introduces a Timer thread in the client, which some may not like. Anything else?

[ron_sigal]

I guess an issue with #2 is it introduces a Timer thread in the client

Currently, each Client creates a new Timer in Client.disconnect(), but I plan (JBREM-1083 "Each Client creates a new invokerDestructionTimer") to make it as static timer.

Anything else?

Setting "idleTimeout" to a positive integer to turn on idle thread harvesting. Note that the number is interpreted as seconds rather than milliseconds. Why? Boh. (http://forum.wordreference.com/showthread.php?t=67620)

-Ron

[ron_sigal]

It occurs to me that each active ServerThread that isn't executing an invocation is sitting in SocketInputStream.read(), and the default timeout is 60000 milliseconds. When the read() times out, the thread should close the socket and return to the threadpool, from which it can be reused by another invocation.

Richard, I'm guessing that the tests you were running were just too short for these threads to time out ... . Does that make sense?

As long as the timeout is that short, then turning on idle thread reaping may not be necessary after all. There's still the issue on the client side of unused sockets sitting around. But as long as EJBs are getting used, the sockets wouldn't go unused. Note that any EJB proxies to the same server should share the same underlying invoker and the same connection pool.

I'm just wondering if UnifiedInvokerProxy was written the way it was, without calling Client.disconnect(), because the typical use case would be to keep making EJB invocations and keep the connections in use.

What about that?

[ron_sigal]

I wrote:

As long as the timeout is that short, then turning on idle thread reaping may not be necessary after all.

Nah, that doesn't make sense. It's true that the sockets would get closed, but the ServerThread would just return to the pool. And it's true that the ServerThread could be reused, but the number of threads would be monotonically non-decreasing, growing as the momentary demand exceeds the number of available threads.

I.e., I'm back in favor of idle thread reaping.

Flippity-flop.

-Ron

[galder.zamarreno]

I'm just wondering if UnifiedInvokerProxy was written the way it was, without calling Client.disconnect(), because the typical use case would be to keep making EJB invocations and keep the connections in use.

I've been wondering about why Tom implemented this in such a way as well but after after looking back at the history of that class, it looks like it's been that way since the very 1st version: http://is.gd/qf2I

I thought it might have been as a result of some perf test or similar but this has been like this since day 1, so I'd imagine it was implemented with the constraints of the first Remoting version integrated in AS.