You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@tomcat.apache.org by ken dombeck <ke...@yahoo.com> on 2012/11/05 19:25:48 UTC

Thread starvation with JDBCStore

Occasionally when a user signs into our web site it is taking a very long time to create a session (10 to 100+ seconds). We traced the issue do to the fact that the background process (ContainerBackgroundProcessor) calls PersistentManagerBase.processExpires() to swap out sessions from memory and remove expired sessions from the database. The issue only appears when we have 10’s of thousands of sessions in memory/database.

The problem is that even though StoreBase.processExpires() correctly releases the locks from JDBCStore (load and remove methods) JDK 6 has implemented “bias locking” http://www.oracle.com/technetwork/java/tuning-139912.html#section4.2.5 which causes this process to ‘hold’ onto the locks obtained in JDBCStore until it is completely done.

It is this thread starvation, when the background process (ContainerBackgroundProcessor) expires/swaps out session runs that other threads are prevented from load sessions from the database due to the locks obtained in JDBCStore.

We are running Tomcat 6.0, OpenJDK 6 (same problem with OpenJDK 7), Ubuntu 10.04.

The problem appears to be a JDK issue that is explained very well in this blog post.
http://ceki.blogspot.com/2009/06/biased-locking-in-java-se-60.html

Reading the blog and looking at the JDK bug tracker it does not appear that the issue will be fixed either.
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4985566

We have come up with a few solutions to this issue that work and would like the community’s input prior to submitting a patch.

1) Connection Pool - currently in Tomcat 6.0 JDBCStore only takes a connectionURL. In Tomcat 7.0 this has been changed to be a connectionURL or dataSourceName. If connectionURL were to be removed from this class and a connection pool were to be used for dataSourceName, then the synchronization could be completely removed from this class. 

Pros: Completely removing the global synchronization improves concurrency which is moved to the JDBC pool implementation instead.
Cons: The main issue with this solution is that it would affect all existing configurations that use connectionURL.

2) Thread priority - By allowing a user to specify the priority of the background process (ContainerBackgroundProcessor) to be a lower priority than other threads the JVM will allow those other threads to obtain the lock occasionally. This did not work as well as the other solutions since the JVM did not relinquish control from the background thread to the other worker threads as often as we would have liked. 

Pros: This would solve any synchronization issues other than just JDBCStore for the entire background process.
Cons: With the background thread set to a lower priority the other worker threads only obtained a time slice ever 3 seconds unlike milliseconds for the other implementations. Thread priority could also be dependent on the JVM implementation/platform.

3) Fair lock - By replacing the synchronization blocks in JDBCStore with java.util.concurrent.locks.ReentrantLock and constructing the lock with fair=true we would allow each thread to get the appropriate time slice. 

Pros: This could be written in a way that would default to fair=false to maintain the default behaviour. A configuration parameter could be added to allow for the setting of the fair flag.
Cons: May be a small performance penalty.

Our proposal is solution #3. 

Let me know your thoughts and we will supply a patch.

Re: Thread starvation with JDBCStore

Posted by ken dombeck <ke...@yahoo.com>.
We will provide the patch for Tomcat 7, is that ok or should it be on master for Tomcat 8?

We won't be updating to Tomcat 7 for a month, so we won't provide the patch until we have fully tested it.

We updated LocalString.properties for our new messages but did not update the other languages since we are not fluent in those. Is that OK?

What should we do about the documentation for JDBCStore? Should we say that it is deprecated and to use DataSourceStore instead? Or leave it and update the documentation to talk about both?

If you are interested you can look at the preliminary changes here.
https://github.com/kdombeck/tomcat70/tree/datasource-session-store



________________________________
 From: Konstantin Kolinko <kn...@gmail.com>
To: Tomcat Users List <us...@tomcat.apache.org> 
Sent: Tuesday, November 6, 2012 4:22 PM
Subject: Re: Thread starvation with JDBCStore
 
2012/11/7 ken dombeck <ke...@yahoo.com>:
> #1 I can definetly create a new store (JDBCRealm, DataSourceRealm), I am just not sure of Apache wanting to support multiple implementations.
>

It is not a big deal. If the new implementation is better we can
deprecate the old one and remove it from future versions. I just do
not see how all issues can be solved just by patching JDBCStore.

For such components the "support" is mostly a supervision. The bug
reports and patches are usually provided by those who are actually
using the component.

> #3 Sorry, I should have mentioned that we tested all 3 options and option 1 and 3 solved our issue. Option 2 helped a bit but was not completely fair to all threads and therefore still allowed the background process to "hold" the lock longer than expected.
>

OK, feel free to submit a patch.

> #4 Increasing the expire frequency is our current short term "hack". This is a hack because it does not remove the problem, it just reduces the frequency of the issue.
>
> #5 I have found several issues opened for the performance issues of expiring database sessions. This could be a future enhancement but does not completely solve this issue.
> https://issues.apache.org/bugzilla/show_bug.cgi?id=34319
> https://issues.apache.org/bugzilla/show_bug.cgi?id=47281
>

BZ 47281 has the same idea that I mentioned,  to use a SELECT WHERE. ;)

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org

Re: Thread starvation with JDBCStore

Posted by Konstantin Kolinko <kn...@gmail.com>.
2012/11/7 ken dombeck <ke...@yahoo.com>:
> #1 I can definetly create a new store (JDBCRealm, DataSourceRealm), I am just not sure of Apache wanting to support multiple implementations.
>

It is not a big deal. If the new implementation is better we can
deprecate the old one and remove it from future versions. I just do
not see how all issues can be solved just by patching JDBCStore.

For such components the "support" is mostly a supervision. The bug
reports and patches are usually provided by those who are actually
using the component.

> #3 Sorry, I should have mentioned that we tested all 3 options and option 1 and 3 solved our issue. Option 2 helped a bit but was not completely fair to all threads and therefore still allowed the background process to "hold" the lock longer than expected.
>

OK, feel free to submit a patch.

> #4 Increasing the expire frequency is our current short term "hack". This is a hack because it does not remove the problem, it just reduces the frequency of the issue.
>
> #5 I have found several issues opened for the performance issues of expiring database sessions. This could be a future enhancement but does not completely solve this issue.
> https://issues.apache.org/bugzilla/show_bug.cgi?id=34319
> https://issues.apache.org/bugzilla/show_bug.cgi?id=47281
>

BZ 47281 has the same idea that I mentioned,  to use a SELECT WHERE. ;)

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org


Re: Thread starvation with JDBCStore

Posted by ken dombeck <ke...@yahoo.com>.
#1 I can definetly create a new store (JDBCRealm, DataSourceRealm), I am just not sure of Apache wanting to support multiple implementations.

#2 I don't think this issue is the cause for the mail thread that you listed. That mail thread does not talk about persisting sessions to the database. But maybe it could be.

#3 Sorry, I should have mentioned that we tested all 3 options and option 1 and 3 solved our issue. Option 2 helped a bit but was not completely fair to all threads and therefore still allowed the background process to "hold" the lock longer than expected.

#4 Increasing the expire frequency is our current short term "hack". This is a hack because it does not remove the problem, it just reduces the frequency of the issue.

#5 I have found several issues opened for the performance issues of expiring database sessions. This could be a future enhancement but does not completely solve this issue.
https://issues.apache.org/bugzilla/show_bug.cgi?id=34319
https://issues.apache.org/bugzilla/show_bug.cgi?id=47281

Thanks for your help. Let me know your thoughts and we will open a ticket in the issue tracker and submit a patch.


________________________________
 From: Konstantin Kolinko <kn...@gmail.com>
To: Tomcat Users List <us...@tomcat.apache.org> 
Sent: Tuesday, November 6, 2012 11:24 AM
Subject: Re: Thread starvation with JDBCStore
 
2012/11/5 ken dombeck <ke...@yahoo.com>:
> Occasionally when a user signs into our web site it is taking a very long time to create a session (10 to 100+ seconds). We traced the issue do to the fact that the background process (ContainerBackgroundProcessor) calls PersistentManagerBase.processExpires() to swap out sessions from memory and remove expired sessions from the database. The issue only appears when we have 10’s of thousands of sessions in memory/database.
>
> The problem is that even though StoreBase.processExpires() correctly releases the locks from JDBCStore (load and remove methods) JDK 6 has implemented “bias locking” http://www.oracle.com/technetwork/java/tuning-139912.html#section4.2.5 which causes this process to ‘hold’ onto the locks obtained in JDBCStore until it is completely done.
>
> It is this thread starvation, when the background process (ContainerBackgroundProcessor) expires/swaps out session runs that other threads are prevented from load sessions from the database due to the locks obtained in JDBCStore.
>
> We are running Tomcat 6.0, OpenJDK 6 (same problem with OpenJDK 7), Ubuntu 10.04.
>
> The problem appears to be a JDK issue that is explained very well in this blog post.
> http://ceki.blogspot.com/2009/06/biased-locking-in-java-se-60.html
>
> Reading the blog and looking at the JDK bug tracker it does not appear that the issue will be fixed either.
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4985566
>
> We have come up with a few solutions to this issue that work and would like the community’s input prior to submitting a patch.
>
> 1) Connection Pool - currently in Tomcat 6.0 JDBCStore only takes a connectionURL. In Tomcat 7.0 this has been changed to be a connectionURL or dataSourceName. If connectionURL were to be removed from this class and a connection pool were to be used for dataSourceName, then the synchronization could be completely removed from this class.
>
> Pros: Completely removing the global synchronization improves concurrency which is moved to the JDBC pool implementation instead.
> Cons: The main issue with this solution is that it would affect all existing configurations that use connectionURL.
>
> 2) Thread priority - By allowing a user to specify the priority of the background process (ContainerBackgroundProcessor) to be a lower priority than other threads the JVM will allow those other threads to obtain the lock occasionally. This did not work as well as the other solutions since the JVM did not relinquish control from the background thread to the other worker threads as often as we would have liked.
>
> Pros: This would solve any synchronization issues other than just JDBCStore for the entire background process.
> Cons: With the background thread set to a lower priority the other worker threads only obtained a time slice ever 3 seconds unlike milliseconds for the other implementations. Thread priority could also be dependent on the JVM implementation/platform.
>
> 3) Fair lock - By replacing the synchronization blocks in JDBCStore with java.util.concurrent.locks.ReentrantLock and constructing the lock with fair=true we would allow each thread to get the appropriate time slice.
>
> Pros: This could be written in a way that would default to fair=false to maintain the default behaviour. A configuration parameter could be added to allow for the setting of the fair flag.
> Cons: May be a small performance penalty.
>
> Our proposal is solution #3.
>
> Let me know your thoughts and we will supply a patch.

1) I'd prefer #1, though it might be easier to implement a separate
class rather than patch this one (like JDBCRealm vs DataSourceRealm).

Currently there are a number of fields in JDBCStore that cannot be
used by more than one thread (e.g. JDBCStore#preparedKeysSql and other
PreparedStatement s there). Such changes will change API, so it is
better to write a separate class.

Disclamer: I have not looked whether such multithreading makes sense
for an org.apache.catalina.Store. I have not reviewed that API.


2) BTW, I wonder whether the following pattern in JDBCStore

        synchronized (this) {
                Connection _conn = getConnection();
        }

may be responsible for some "Sessions are not expiring" threads that I
see on the users list. E.g.
http://markmail.org/thread/jkbbiqpxodefpm6i

(Though we are still waiting for a thread dump in that thread, so
currently it is not known what the problem is).

A common mistake with DBCP pool is to configure an infinite timeout there.


3) Have you tested that #3 solves your problem?

IMO, changing synchronization object is a slight API change (from the
POV of child classes). I would not object to it though if it solves a
real problem and the lock object is available to the child classes.

4) It is possible to configure the background thread to run more rarely.

It is also possible to configure separate background thread per
container. You can have a separate thread for your webapp.

It is also possible to configure "processExpiresFrequency" attribute
on a <Manager>.

5) I wonder whether it is possible to implement expiration in
JDBCStore more effectively.

Couldn't expired sessions be selected with one SQL statement, instead
of iterating over the all of them and loading them one-by-one? That is
what the default implementation in StoreBase#processExpires() does.

There are already separate "sessionLastAccessedCol" and
"sessionMaxInactiveCol" columns in the JDBCStore configuration.


Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org

Re: Thread starvation with JDBCStore

Posted by Konstantin Kolinko <kn...@gmail.com>.
2012/11/5 ken dombeck <ke...@yahoo.com>:
> Occasionally when a user signs into our web site it is taking a very long time to create a session (10 to 100+ seconds). We traced the issue do to the fact that the background process (ContainerBackgroundProcessor) calls PersistentManagerBase.processExpires() to swap out sessions from memory and remove expired sessions from the database. The issue only appears when we have 10’s of thousands of sessions in memory/database.
>
> The problem is that even though StoreBase.processExpires() correctly releases the locks from JDBCStore (load and remove methods) JDK 6 has implemented “bias locking” http://www.oracle.com/technetwork/java/tuning-139912.html#section4.2.5 which causes this process to ‘hold’ onto the locks obtained in JDBCStore until it is completely done.
>
> It is this thread starvation, when the background process (ContainerBackgroundProcessor) expires/swaps out session runs that other threads are prevented from load sessions from the database due to the locks obtained in JDBCStore.
>
> We are running Tomcat 6.0, OpenJDK 6 (same problem with OpenJDK 7), Ubuntu 10.04.
>
> The problem appears to be a JDK issue that is explained very well in this blog post.
> http://ceki.blogspot.com/2009/06/biased-locking-in-java-se-60.html
>
> Reading the blog and looking at the JDK bug tracker it does not appear that the issue will be fixed either.
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4985566
>
> We have come up with a few solutions to this issue that work and would like the community’s input prior to submitting a patch.
>
> 1) Connection Pool - currently in Tomcat 6.0 JDBCStore only takes a connectionURL. In Tomcat 7.0 this has been changed to be a connectionURL or dataSourceName. If connectionURL were to be removed from this class and a connection pool were to be used for dataSourceName, then the synchronization could be completely removed from this class.
>
> Pros: Completely removing the global synchronization improves concurrency which is moved to the JDBC pool implementation instead.
> Cons: The main issue with this solution is that it would affect all existing configurations that use connectionURL.
>
> 2) Thread priority - By allowing a user to specify the priority of the background process (ContainerBackgroundProcessor) to be a lower priority than other threads the JVM will allow those other threads to obtain the lock occasionally. This did not work as well as the other solutions since the JVM did not relinquish control from the background thread to the other worker threads as often as we would have liked.
>
> Pros: This would solve any synchronization issues other than just JDBCStore for the entire background process.
> Cons: With the background thread set to a lower priority the other worker threads only obtained a time slice ever 3 seconds unlike milliseconds for the other implementations. Thread priority could also be dependent on the JVM implementation/platform.
>
> 3) Fair lock - By replacing the synchronization blocks in JDBCStore with java.util.concurrent.locks.ReentrantLock and constructing the lock with fair=true we would allow each thread to get the appropriate time slice.
>
> Pros: This could be written in a way that would default to fair=false to maintain the default behaviour. A configuration parameter could be added to allow for the setting of the fair flag.
> Cons: May be a small performance penalty.
>
> Our proposal is solution #3.
>
> Let me know your thoughts and we will supply a patch.

1) I'd prefer #1, though it might be easier to implement a separate
class rather than patch this one (like JDBCRealm vs DataSourceRealm).

Currently there are a number of fields in JDBCStore that cannot be
used by more than one thread (e.g. JDBCStore#preparedKeysSql and other
PreparedStatement s there). Such changes will change API, so it is
better to write a separate class.

Disclamer: I have not looked whether such multithreading makes sense
for an org.apache.catalina.Store. I have not reviewed that API.


2) BTW, I wonder whether the following pattern in JDBCStore

        synchronized (this) {
                Connection _conn = getConnection();
        }

may be responsible for some "Sessions are not expiring" threads that I
see on the users list. E.g.
http://markmail.org/thread/jkbbiqpxodefpm6i

(Though we are still waiting for a thread dump in that thread, so
currently it is not known what the problem is).

A common mistake with DBCP pool is to configure an infinite timeout there.


3) Have you tested that #3 solves your problem?

IMO, changing synchronization object is a slight API change (from the
POV of child classes). I would not object to it though if it solves a
real problem and the lock object is available to the child classes.

4) It is possible to configure the background thread to run more rarely.

It is also possible to configure separate background thread per
container. You can have a separate thread for your webapp.

It is also possible to configure "processExpiresFrequency" attribute
on a <Manager>.

5) I wonder whether it is possible to implement expiration in
JDBCStore more effectively.

Couldn't expired sessions be selected with one SQL statement, instead
of iterating over the all of them and loading them one-by-one? That is
what the default implementation in StoreBase#processExpires() does.

There are already separate "sessionLastAccessedCol" and
"sessionMaxInactiveCol" columns in the JDBCStore configuration.


Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org