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.

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

Jetty 9.4.x #1970 lost selector #2023

Closed
wants to merge 7 commits into from

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Dec 6, 2017

Issue #1970

sbordet and others added 4 commits November 20, 2017 22:03
Instrumented EWYK to gather additional information
in case the selector thread is lost.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Extended LowResourceMonitor to also check selector health
Improved the dump of ManagedSelector and EWYK

Signed-off-by: Greg Wilkins <gregw@webtide.com>
The actions should not be none blocking and they are intended to be executed
by the selector thread between selections, so instead of passing them back
to an execution strategy that might dispatch them, execute them in a tight
loop with lock held.  If a blocking task is discovered, warn and then pass
back to the exection strategy.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Dec 6, 2017

@sbordet I've made this PR for:

  • our original changes in the ManagedSelector can lose selector thread under high concurrent load #1970 branch that cleaned up the EWYK loop
  • an extension to the LowResourcesMonitor to check the health of selectors and to dump them if they do not respond
  • improvements to the dump of SelectorManager and EWYK
  • the ability to dump some history of EWYK with -Dorg.eclipse.jetty.util.thread.strategy.EatWhatYouKill.HISTORY=true
  • a change to SelectorManager to run actions in a tight loop, as they should not be given to an executionStrategy 40c5826

@gregw
Copy link
Contributor Author

gregw commented Dec 11, 2017

@sbordet the ip-validation is red because I did a merge and it's commits don't have signed-off-by clauses and the ip system is too stupid to notice that the author is a committer anyway. We can ignore the red X and merge anyway (when ready) as the committer who does the merge is responsible for checking anyway (and if we squash they make the commit).

@Test
public void testStuckSelector() throws Exception
{
// TODO currently this is test needs human inspection of the log output
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbordet Not sure if we should keep this test, as it is has no asserts and needs visual confirmation.

private final LongAdder _nonBlocking = new LongAdder();
private final LongAdder _blocking = new LongAdder();
private final LongAdder _executed = new LongAdder();
private final Producer _producer;
private final Executor _executor;
private final ReservedThreadExecutor _producers;
private State _state = State.IDLE;
private Thread _producerThread;
private int _ancient;
private int _history;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbordet I propose that we remove the _ancient and _history fields and the static array of state strings

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbordet looking at this again, I think the JIT will take out all the history stuff, so I think this is good to merge as is!

@gregw gregw added the Sponsored This issue affects a user with a commercial support agreement label Dec 11, 2017
@gregw
Copy link
Contributor Author

gregw commented Dec 27, 2017

replaced by #2062 and #2063

@gregw gregw closed this Dec 27, 2017
@gregw gregw deleted the jetty-9.4.x-1970-lost_selector branch January 8, 2018 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants