Fix defect in DoSFilter where semaphore acquires my be leaked #54
Conversation
When trying to release the semaphore in the finally block, 'asyncContext.dispatch()' may throw a "RejectedExecutionException". If this occurs, then the semaphore will never be released. Ultimately the condition will result in all threads blocking to acquire the semaphore as the DoSFilter is continue to be used.
|
I am not sure where it is getting my fullcontact email from. I committed with the same email email I registered on the foundation and signed the agreement with. |
|
Your email address is likely coming from your github credentials. |
|
Looking at the raw commit for this PR, it shows your gmail address. |
|
Something is wrong with the CLA check webhook. |
|
I've posted a response on the bug. |
|
@eclipsewebmaster I have both emails associated with my github account. The jentfoo email is my primary github email, and the fullcontact one is a secondary one, associated only to the fullcontact organization in github. In any case, both emails are associated to github. |
|
Sorry, I just checked, another email is primary (which is private). But the jentfoo email is still associated with my github like I originally indicated. |
|
@jentfoo This change is good..... however I would say that if AsyncContext.dispatch() is throwing RejectedExecutionException then your service is going to have other problems and failed requests. The idea of the DoS filter is to limit resource commitments to what the server has on offer, so in this case either your threadpool is too small, your threadpool Q is too finite, there are too many other requests not going via the DoSFilter or the DoSFilter limit is set too high. So it would be good to know if you can configure to avoid this exception (but happy to have the finally to handle it if it does happen). |
|
@gregw thanks for the response. I am aware of this. This is an internal service, and we have already made the decision that we would rather fail fast, rather than add latency to these requests (and thus why we have a very limited queue to the pool). You also hit the nail on the head when you said "there are too many other requests not going via the DoSFilter". We are using this dos filter to actually protect a specific database from becoming overburdened. And are actually only filtering a few specific user agents (aka other internal services)...most requests are ignored by this filter. I am aware that if the pool queue was large enough, in theory this may not be happening. But even in that condition this still would be theoretically possible given enough load/queued requests. |
|
@jentfoo Mike thanks for the extra information. Note that historically jetty was a bit fragile to a limited threadpool and rejected execution failures, however we believe that with 9.3.x and our Eat-What-You-Kill scheduling, then we are more robust in the advent of such failures as Dispatches are only used to recruit more helpers and that even if they fail a single thread could actually perform all the tasks serially and then go back to selecting for more. While testing this limited threadpool scenario, I'd be keen to know of any other issues that you may detect. So we'll merge this one once the ip-validation is sorted. @joakime are you on that case? |
|
on the ip-validation front ... Updated message from Denis Roy...
@jentfoo I think you'll want to correct that for future commits. @gregw I would expect we are clear, as his |
|
merged to 9.3.x - thanks! |
When trying to release the semaphore in the finally block, 'asyncContext.dispatch()' may throw a "RejectedExecutionException". If this occurs, then the semaphore will never be released. Ultimately the condition will result in all threads blocking to acquire the semaphore as the DoSFilter is continue to be used. #54 Signed-off-By: jentfoo@gmail.com
When trying to release the semaphore in the finally block, 'asyncContext.dispatch()' may throw a "RejectedExecutionException". If this occurs, then the semaphore will never be released. Ultimately the condition will result in all threads blocking to acquire the semaphore as the DoSFilter is continue to be used. #54 Signed-off-By: jentfoo@gmail.com
When trying to release the semaphore in the finally block, 'asyncContext.dispatch()' may throw a "RejectedExecutionException". If this occurs, then the semaphore will never be released.
Ultimately the condition will result in all threads blocking to acquire the semaphore as the DoSFilter is continue to be used.