From 333c22d670fd35a86bb311f1d81dff1dc6427a5e Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 30 Oct 2017 00:36:01 +0100 Subject: [PATCH] Issue #1924 - ManagedSelector livelock. * Actually using the MAX_ACTION_PERIOD value rather than a harcoded one. * Waking up the selector outside the sync block. --- .../eclipse/jetty/client/LivelockTest.java | 42 +++++++++++------ .../org/eclipse/jetty/io/ManagedSelector.java | 45 +++++++++++-------- 2 files changed, 54 insertions(+), 33 deletions(-) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/LivelockTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/LivelockTest.java index 71dc8c0e932..fca2d1f2074 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/LivelockTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/LivelockTest.java @@ -25,18 +25,15 @@ import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.jetty.client.http.HttpClientTransportOverHTTP; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.io.ManagedSelector; -import org.eclipse.jetty.io.SelectorManager; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; -import org.eclipse.jetty.toolchain.test.annotation.Slow; import org.eclipse.jetty.util.SocketAddressResolver; import org.eclipse.jetty.util.thread.Invocable; import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.After; import org.junit.Assert; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; public class LivelockTest @@ -61,19 +58,20 @@ public class LivelockTest @After public void after() throws Exception { - client.stop(); - server.stop(); + if (client != null) + client.stop(); + if (server != null) + server.stop(); } @Test - @Slow public void testLivelock() throws Exception { // This test applies a moderate connect/request load (5/s) over 5 seconds, - // with a connect timeout of 100, so any delayed connects will be detected. - // NonBlocking runnables are submitted to both the client and server + // with a connect timeout of 1000, so any delayed connects will be detected. + // NonBlocking actions are submitted to both the client and server // ManagedSelectors that submit themselves in an attempt to cause a live lock - // as there will always be an action available to run + // as there will always be an action available to run. int count = 25; HttpClientTransport transport = new HttpClientTransportOverHTTP(1); @@ -96,7 +94,7 @@ public class LivelockTest @Override public void run() { - try { Thread.sleep(10);} catch(Exception e) {} + sleep(10); if (busy.get()) clientSelector.submit(this); } @@ -109,13 +107,15 @@ public class LivelockTest @Override public void run() { - try { Thread.sleep(10);} catch(Exception e) {} + sleep(10); if (busy.get()) serverSelector.submit(this); } }; serverSelector.submit(serverLivelock); - + + int requestRate = 5; + long pause = 1000 / requestRate; CountDownLatch latch = new CountDownLatch(count); for (int i = 0; i < count; ++i) { @@ -126,9 +126,23 @@ public class LivelockTest if (result.isSucceeded() && result.getResponse().getStatus() == HttpStatus.OK_200) latch.countDown(); }); - Thread.sleep(200); + sleep(pause); } - Assert.assertTrue(latch.await(5000, TimeUnit.MILLISECONDS)); + Assert.assertTrue(latch.await(2 * pause * count, TimeUnit.MILLISECONDS)); + + // Exit the livelocks. busy.set(false); } + + private void sleep(long time) + { + try + { + Thread.sleep(time); + } + catch (InterruptedException x) + { + throw new RuntimeException(x); + } + } } diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ManagedSelector.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ManagedSelector.java index 1d85e72a0c8..c2e410c7091 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/ManagedSelector.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/ManagedSelector.java @@ -69,7 +69,6 @@ public class ManagedSelector extends ContainerLifeCycle implements Dumpable private Selector _selector; private long _actionTime = -1; - public ManagedSelector(SelectorManager selectorManager, int id) { _selectorManager = selectorManager; @@ -305,11 +304,11 @@ public class ManagedSelector extends ContainerLifeCycle implements Dumpable private Runnable nextAction() { - Runnable action; long now = System.nanoTime(); + Selector selector = null; + Runnable action = null; try (Locker.Lock lock = _locker.lock()) { - // It is important to avoid live-lock (busy blocking) here. If too many actions // are submitted, this can indefinitely defer selection happening. Similarly if // we give too much priority to selection, it may prevent actions from being run. @@ -317,30 +316,38 @@ public class ManagedSelector extends ContainerLifeCycle implements Dumpable // so that this method will fall through to selection if more than MAX_ACTION_PERIOD_MS // is spent running actions. The time period is cleared whenever a selection occurs, // so that a full period can be spent on actions after every select. - - if (_actionTime==-1) - _actionTime = now; - else if ((now-_actionTime)>TimeUnit.MILLISECONDS.toNanos(100) && _actions.size()>0) + + if (_actionTime == -1) { - // Too long spent handling actions, give selection a go by, - // immediate wakeup (as if remaining action were just added). - _selector.wakeup(); + _actionTime = now; + } + else if ((now - _actionTime) > TimeUnit.MILLISECONDS.toNanos(MAX_ACTION_PERIOD_MS) && _actions.size() > 0) + { + // Too much time spent handling actions, give selection a go, + // immediately waking up (as if remaining action were just added). + selector = _selector; _selecting = false; _actionTime = -1; if (LOG.isDebugEnabled()) - LOG.debug("force select q={}",_actions.size()); - return null; + LOG.debug("Forcing selection, actions={}",_actions.size()); } - - action = _actions.poll(); - if (action == null) + + if (selector == null) { - // No more actions, so we time to do some selecting - _selecting = true; - _actionTime = -1; + action = _actions.poll(); + if (action == null) + { + // No more actions, so we time to do some selecting + _selecting = true; + _actionTime = -1; + } } - return action; } + + if (selector != null) + selector.wakeup(); + + return action; } private boolean select()