Quantcast

[jira] (JETTY-1527) QueuedThreadPool Anomaly

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[jira] (JETTY-1527) QueuedThreadPool Anomaly

JIRA jira@codehaus.org
Issue Type: Bug Bug
Assignee: Unassigned
Attachments: ContrivedQueuedThreadPoolTest.java
Created: 20/Jun/12 6:24 PM
Description:

Under a quite contrived unit test scenario, discovered that the QueuedThreadPool thread pool in Jetty util can be reluctant to grow when it should....

In the attached very contrived unit test, a pool is configured with a maximum of 50 threads. The test then creates ten runables which are given to the pool to execute. However, only the first nine of those runnables actually start running and so the test will fail at the following assert:-

Assert.assertEquals(testThreadsRunning, TEST_THREADS);

The tenth runnable is left in the queue until such time as a thread returns to the pool (which in this contrived test, they don't).

In normal running, there are continually threads being put back into the pool such that if the pool should have/could have grown, a request doesn't normally have to wait long to get picked up.

I think the pool here is too reluctant to grow which means that sometimes requests in Jetty maybe subject to addition latency in processing.

I think the fix is to move a couple of lines in the dispatch method, but I don't know if this might make the pool to eager to grow....

public boolean dispatch(Runnable job)
{
if (isRunning())
{
if (_jobs.offer(job))
{
final int jobQ = _jobs.size();
final int idle = getIdleThreads();

// If we had no idle threads or the jobQ is greater than the idle threads
if (idle == 0 || jobQ > idle)

{ int threads = _threadsStarted.get(); if (threads <= _maxThreads) startThread(threads); }

return true;
}
else
{
LOG.debug("Declined {} on {}", job, this);
}
}
LOG.debug("Dispatched {} to stopped {}", job, this);
return false;
}

Project: Jetty
Priority: Minor Minor
Reporter: David Wade
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators.
For more information on JIRA, see: http://www.atlassian.com/software/jira
--------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[jira] (JETTY-1527) QueuedThreadPool Anomaly

JIRA jira@codehaus.org
Joakim Erdfelt commented on Bug JETTY-1527

Which version of jetty?

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators.
For more information on JIRA, see: http://www.atlassian.com/software/jira
--------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[jira] (JETTY-1527) QueuedThreadPool Anomaly

JIRA jira@codehaus.org
In reply to this post by JIRA jira@codehaus.org
David Wade commented on Bug JETTY-1527

This test was on 7.2. I noticed this on 7.4. Just reran against 7.6.0.v20120127 which failed also.

Oh, yes, the parameters passed on the fail assert are the wrong way round... The expected should be 10 TEST_THREADS). The actual value I get on 7.6 is 8. On 7.2 it was 9... So the issue got worse perhaps.

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators.
For more information on JIRA, see: http://www.atlassian.com/software/jira
--------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[jira] (JETTY-1527) QueuedThreadPool Anomaly

JIRA jira@codehaus.org
In reply to this post by JIRA jira@codehaus.org
David Wade commented on Bug JETTY-1527

Actually, after taking a look at the existing dispatch method, I don't know why the logic contains an OR such that a thread is created when there are no idle threads... Surely the only issue is, are there more items in the queue then we have idle threads to process them; in which case the dispatch method should change to something like this (which passes my contrived test).

public boolean dispatch(Runnable job)
{
if (isRunning())
{
if (_jobs.offer(job))
{
final int jobQ = _jobs.size();
final int idle = getIdleThreads();

// If is greater than the idle threads
if (jobQ > idle)

{ int threads = _threadsStarted.get(); if (threads <= _maxThreads) startThread(threads); }

return true;
}
else
{
LOG.debug("Declined {} on {}", job, this);
}
}
LOG.debug("Dispatched {} to stopped {}", job, this);
return false;
}

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators.
For more information on JIRA, see: http://www.atlassian.com/software/jira
--------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[jira] (JETTY-1527) QueuedThreadPool Anomaly

JIRA jira@codehaus.org
In reply to this post by JIRA jira@codehaus.org
Jan Bartel commented on Bug JETTY-1527

Hi David,

Thanks for providing the test case. Looking at it, the QueuedThreadPool no doubt has a few funnies in it - for example getting the jobs size and the number of idle threads aren't synchronized in any way. Probably moving those 2 lines down to after the call to insert a new job in the queue changes the timing a bit and your test happens to work. I can also get it to work if I insert a little pause after each call to pool.execute() in the test.

We're a bit reluctant to make changes to the QueuedThreadPool class at this point, as its a highly concurrent class and changes could introduce all sorts of subtle effects, and additionally, for jetty-9, we're probably ditching it in favour of a disruptor style mechanism.

thanks
Jan

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators.
For more information on JIRA, see: http://www.atlassian.com/software/jira
--------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[jira] (JETTY-1527) QueuedThreadPool Anomaly

JIRA jira@codehaus.org
In reply to this post by JIRA jira@codehaus.org
David Wade commented on Bug JETTY-1527

Thanks Jan,

We actually noticed this problem in our running application on Jetty (with commercial support from Intalio). However, this was not when used with the HTTP connector but when reusing for our own thread pools within our applications (so not a support issue). Our applications are running on multiple reasonably big servers (24 actual cores per server). Within our applications, we use the pool patched.

A bit more on the current implementation, those two lines are currently before the potentially blocking call to _jobs.offer(job). This means on heavily used and threaded applications, in computing terms there can be a large amount of time between the snapshot of those values and making the decision as to whether to grow the pool or not. If the snapshot is performed after the call there is a very compute small time inbetween (baring timeslicing). But more importantly, there is practically no chance the pool will have failed to grow when it needed too.

I understand the reluctance to fix/change, but suspect there may be more than subtle effects when changing to disruptors. In the meantime, this leaves a potential erratic latancy issue for Jetty users unresolved, perhaps for years before they can upgrade to 9.1...

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators.
For more information on JIRA, see: http://www.atlassian.com/software/jira
--------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[jira] (JETTY-1527) QueuedThreadPool Anomaly

JIRA jira@codehaus.org
In reply to this post by JIRA jira@codehaus.org

If you like, ping us through the normal commercial support channels to discuss as well. I am not sure I would classify it as out of band for support. Obviously we would update here moving forward but please free to ping us there as well.

cheers,
jesse

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators.
For more information on JIRA, see: http://www.atlassian.com/software/jira
--------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[jira] (JETTY-1527) QueuedThreadPool Anomaly

JIRA jira@codehaus.org
In reply to this post by JIRA jira@codehaus.org
Greg Wilkins commented on Bug JETTY-1527

David,

I will have a look at changing this, but the point to note about this code is not that it "has a few funnies" (thank you Jan, but that it is written to be concurrent and probabilistic. Ie growing the thread pool is not deterministic. We have to strike a balance between using existing threads vs creating new ones and we have to be careful that we don't drive the system to create more threads than it needs.

But I'll have a look and see if we can make improve this situation.... but not promising anything yet.

PS. just because you have plugged in your own thread pool does not mean you can't raise issues via your commercial support arrangements. Jetty is designed to be extended and replugged, so if we used every such change as a reason to not support jetty then we'd have a very quiet life.

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators.
For more information on JIRA, see: http://www.atlassian.com/software/jira
--------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[jira] (JETTY-1527) QueuedThreadPool Anomaly

JIRA jira@codehaus.org
In reply to this post by JIRA jira@codehaus.org
Change By: Greg Wilkins (03/Jul/12 3:37 AM)
Assignee: Greg Wilkins
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators.
For more information on JIRA, see: http://www.atlassian.com/software/jira
--------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email
Loading...