Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve insufficient thread warnings/errors #1851

Closed
gregw opened this issue Sep 27, 2017 · 6 comments
Closed

Improve insufficient thread warnings/errors #1851

gregw opened this issue Sep 27, 2017 · 6 comments
Assignees
@gregw @sbordet
Labels
Enhancement Sponsored This issue affects a user with a commercial support agreement
Milestone
9.4.x

Comments

@gregw
Copy link
Contributor

gregw commented Sep 27, 2017

If a server or client is configured with a threadpool containing insufficient threads, then an informative warning and/or error should occur
gregw added a commit that referenced this issue Sep 27, 2017
@gregw
Issue #1851 Improve insufficient thread warnings/errors
abc5eac 
Created a ThreadBudget class that can be used to warn/error for
registered and unregistered allocations of threads.

The server on doStart does an unregistered check of all its components that
implement the Allocation interface.

The client will register itself as an Allocation if a shared Executor is used.
@gregw
Copy link
Contributor Author

gregw commented Sep 27, 2017

Branch jetty-9.4.x-1851-ThreadBudget has an improved thread usage warning. Instead of a heuristic hard coded in the server, there is now a ThreadBudget.Allocation interface that components can implement to declare how many threads they will need.

@gregw gregw added this to the 9.4.x milestone Sep 27, 2017
@gregw
Copy link
Contributor Author

gregw commented Sep 27, 2017

@sbordet review needed, specially on the client side.

gregw added a commit that referenced this issue Sep 27, 2017
@gregw
Issue #1851 Improve insufficient thread warnings/errors
8bcc460 
Refactor approach to use Leases, to handle multiple executors
gregw added a commit that referenced this issue Sep 27, 2017
@gregw
Issue #1851 improved test
a63894d
gregw added a commit that referenced this issue Sep 27, 2017
@gregw
Issue #1851 added reset
f962f18
@gregw
Copy link
Contributor Author

gregw commented Sep 27, 2017

@sbordet I had to take an entirely different approach. There are now leases on threads taken to express commitments.

gregw added a commit that referenced this issue Sep 27, 2017
@gregw
Issue #1851 Improve insufficient thread warnings/errors
7d98cbb 
Squashed commit of the following:

commit 1d9e8e4
Merge: 7280594 55b0f10
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Sep 28 07:20:37 2017 +1000

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-1851-ThreadBudget

commit 7280594
Author: Greg Wilkins <gregw@webtide.com>
Date:   Wed Sep 27 22:48:58 2017 +1000

    fixed headers

commit f962f18
Author: Greg Wilkins <gregw@webtide.com>
Date:   Wed Sep 27 18:12:33 2017 +1000

    Issue #1851 added reset

commit a63894d
Author: Greg Wilkins <gregw@webtide.com>
Date:   Wed Sep 27 18:08:53 2017 +1000

    Issue #1851 improved test

commit 8bcc460
Author: Greg Wilkins <gregw@webtide.com>
Date:   Wed Sep 27 18:03:47 2017 +1000

    Issue #1851 Improve insufficient thread warnings/errors

    Refactor approach to use Leases, to handle multiple executors

commit fe4be5f
Merge: abc5eac a248d38
Author: Greg Wilkins <gregw@webtide.com>
Date:   Wed Sep 27 15:37:56 2017 +1000

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-1851-ThreadBudget

commit abc5eac
Author: Greg Wilkins <gregw@webtide.com>
Date:   Wed Sep 27 12:20:03 2017 +1000

    Issue #1851 Improve insufficient thread warnings/errors

    Created a ThreadBudget class that can be used to warn/error for
    registered and unregistered allocations of threads.

    The server on doStart does an unregistered check of all its components that
    implement the Allocation interface.

    The client will register itself as an Allocation if a shared Executor is used.
@sbordet
Copy link
Contributor

sbordet commented Sep 29, 2017

@gregw I think we must also call leaseTo() from the SelectorManager for the number of selectors.

These threads will be blocked in select() and therefore unavailable, especially in a shared thread pool scenario (e.g. one HttpClient is completely idle, but "steals" a number of selector threads, while another HttpClient is active and will not have enough threads).

Also, should Server lease one thread ? The minimal case would be 1 acceptor, 1 selector, 0 reserved, but there must be one more thread to run the request.

Furthermore, I'd like to rename ThreadPool.SizedThreadPool to just ThreadPool.Sized and ThreadBudget to ThreadPoolBudget.

@gregw
Copy link
Contributor Author

gregw commented Oct 1, 2017

Good point on the selectors....
I'm neutral on the name changes... they are better names, but perhaps only rename in jetty-10?

Note that minimal is 0 acceptors, as we can do async accepting.

I don't think the Server should lease a thread, as we have the warnAt margin. If you have < warnAt (defaults to number of CPUs) then you get a warning, which becomes an error at 0. So if no threads available to handle requests, that is already a hard error

sbordet added a commit that referenced this issue Oct 2, 2017
@sbordet
Issue #1851 - Improve insufficient thread warnings/errors.
3b98a6c 
ThreadBudget -> ThreadPoolBudget.
Added selectors to the leased threads.
@sbordet
Copy link
Contributor

sbordet commented Oct 2, 2017

@gregw I added leasing on selectors and renamed ThreadBudget to ThreadPoolBudget (which is a newly introduced class never released and does not need deprecation).
Added a test case for multiple HttpClients sharing the same ThreadPool.

@sbordet sbordet mentioned this issue Oct 6, 2017
Closed
sbordet added a commit that referenced this issue Oct 6, 2017
@sbordet
Issue #1851 - Improve insufficient thread warnings/errors.
c93360a 
Added ExecutorSizedThreadPool, a wrapper around JDK's ThreadPoolExecutor
that implements SizedThreadPool (and therefore returns a ThreadPoolBudget).

Deprecated ExecutorThreadPool, an older version of ExecutorSizedThreadPool.
@gregw gregw added the Sponsored This issue affects a user with a commercial support agreement label Oct 12, 2017
@gregw gregw closed this as completed Oct 12, 2017
@joakime joakime mentioned this issue Jan 15, 2018
Closed
@joakime joakime mentioned this issue Aug 13, 2018
Closed
@gregw gregw mentioned this issue Oct 8, 2019
Closed
@dhorn712 dhorn712 mentioned this issue Aug 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gregw gregw

@sbordet sbordet

Labels
Enhancement Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Milestone
9.4.x
Development

No branches or pull requests
2 participants
@gregw @sbordet