From cf418c639f7fa75fec8560f92806205d829f6dae Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 10 Aug 2017 12:52:28 +1000 Subject: [PATCH] Revert "added assume from running directly" This reverts commit 39ea66368ce28f6219dbef386a857e46454b7b95. --- .../jetty/server/AbstractConnector.java | 99 ++++++------------- .../server/AbstractNetworkConnector.java | 11 ++- .../eclipse/jetty/server/ServerConnector.java | 5 +- .../org/eclipse/jetty/util/TypeUtilTest.java | 5 +- 4 files changed, 42 insertions(+), 78 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java index f9aff946ed6..232f96ac97f 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java @@ -21,7 +21,6 @@ package org.eclipse.jetty.server; import java.io.IOException; import java.net.Socket; import java.nio.ByteBuffer; -import java.nio.channels.ClosedByInterruptException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -35,7 +34,6 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executor; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; -import java.util.concurrent.locks.Condition; import org.eclipse.jetty.io.ArrayByteBufferPool; import org.eclipse.jetty.io.ByteBufferPool; @@ -50,7 +48,6 @@ import org.eclipse.jetty.util.component.Dumpable; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.ssl.SslContextFactory; -import org.eclipse.jetty.util.thread.Locker; import org.eclipse.jetty.util.thread.ScheduledExecutorScheduler; import org.eclipse.jetty.util.thread.Scheduler; @@ -140,10 +137,8 @@ import org.eclipse.jetty.util.thread.Scheduler; public abstract class AbstractConnector extends ContainerLifeCycle implements Connector, Dumpable { protected final Logger LOG = Log.getLogger(AbstractConnector.class); - - private final Locker _locker = new Locker(); - private final Condition _setAccepting = _locker.newCondition(); - private final Map _factories = new LinkedHashMap<>(); // Order is important on server side, so we use a LinkedHashMap + // Order is important on server side, so we use a LinkedHashMap + private final Map _factories = new LinkedHashMap<>(); private final Server _server; private final Executor _executor; private final Scheduler _scheduler; @@ -151,13 +146,12 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co private final Thread[] _acceptors; private final Set _endpoints = Collections.newSetFromMap(new ConcurrentHashMap<>()); private final Set _immutableEndPoints = Collections.unmodifiableSet(_endpoints); - private CountDownLatch _stopping; + private volatile CountDownLatch _stopping; private long _idleTimeout = 30000; private String _defaultProtocol; private ConnectionFactory _defaultConnectionFactory; private String _name; private int _acceptorPriorityDelta=-2; - private boolean _accepting = true; /** @@ -289,7 +283,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co protected void interruptAcceptors() { - try (Locker.Lock lock = _locker.lockIfNotHeld()) + synchronized (this) { for (Thread thread : _acceptors) { @@ -333,7 +327,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co public void join(long timeout) throws InterruptedException { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { for (Thread thread : _acceptors) if (thread != null) @@ -348,31 +342,15 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co /** * @return Is the connector accepting new connections */ - public boolean isAccepting() + protected boolean isAccepting() { - try (Locker.Lock lock = _locker.lock()) - { - return _accepting; - } + return isRunning(); } - public void setAccepting(boolean accepting) - { - try (Locker.Lock lock = _locker.lock()) - { - _accepting=accepting; - if (accepting) - _setAccepting.signalAll(); - else - interruptAcceptors(); - } - } - - @Override public ConnectionFactory getConnectionFactory(String protocol) { - try (Locker.Lock lock = _locker.lock()) + synchronized (_factories) { return _factories.get(StringUtil.asciiToLowerCase(protocol)); } @@ -381,7 +359,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co @Override public T getConnectionFactory(Class factoryType) { - try (Locker.Lock lock = _locker.lock()) + synchronized (_factories) { for (ConnectionFactory f : _factories.values()) if (factoryType.isAssignableFrom(f.getClass())) @@ -392,7 +370,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co public void addConnectionFactory(ConnectionFactory factory) { - try (Locker.Lock lock = _locker.lock()) + synchronized (_factories) { Set to_remove = new HashSet<>(); for (String key:factory.getProtocols()) @@ -431,7 +409,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co public void addFirstConnectionFactory(ConnectionFactory factory) { - try (Locker.Lock lock = _locker.lock()) + synchronized (_factories) { List existings = new ArrayList<>(_factories.values()); _factories.clear(); @@ -444,7 +422,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co public void addIfAbsentConnectionFactory(ConnectionFactory factory) { - try (Locker.Lock lock = _locker.lock()) + synchronized (_factories) { String key=StringUtil.asciiToLowerCase(factory.getProtocol()); if (_factories.containsKey(key)) @@ -466,7 +444,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co public ConnectionFactory removeConnectionFactory(String protocol) { - try (Locker.Lock lock = _locker.lock()) + synchronized (_factories) { ConnectionFactory factory= _factories.remove(StringUtil.asciiToLowerCase(protocol)); removeBean(factory); @@ -477,7 +455,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co @Override public Collection getConnectionFactories() { - try (Locker.Lock lock = _locker.lock()) + synchronized (_factories) { return _factories.values(); } @@ -485,7 +463,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co public void setConnectionFactories(Collection factories) { - try (Locker.Lock lock = _locker.lock()) + synchronized (_factories) { List existing = new ArrayList<>(_factories.values()); for (ConnectionFactory factory: existing) @@ -560,23 +538,14 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co return getConnectionFactory(_defaultProtocol); } - protected boolean handleAcceptFailure(Throwable ex) + protected boolean handleAcceptFailure(Throwable previous, Throwable current) { - if (isRunning()) + if (isAccepting()) { - if (ex instanceof InterruptedException) - { - LOG.debug(ex); - return true; - } - - if (ex instanceof ClosedByInterruptException) - { - LOG.debug(ex); - return false; - } - - LOG.warn(ex); + if (previous == null) + LOG.warn(current); + else + LOG.debug(current); try { // Arbitrary sleep to avoid spin looping. @@ -587,13 +556,12 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co } catch (Throwable x) { - LOG.ignore(x); + return false; } - return false; } else { - LOG.ignore(ex); + LOG.ignore(current); return false; } } @@ -627,28 +595,19 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co try { - while (isRunning()) + Throwable exception = null; + while (isAccepting()) { - try (Locker.Lock lock = _locker.lock()) - { - if (!_accepting) - { - _setAccepting.await(); - continue; - } - } - catch (InterruptedException e) - { - continue; - } - try { accept(_id); + exception = null; } catch (Throwable x) { - if (!handleAcceptFailure(x)) + if (handleAcceptFailure(exception, x)) + exception = x; + else break; } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractNetworkConnector.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractNetworkConnector.java index 3a280cf1371..76bd05ee2c0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractNetworkConnector.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractNetworkConnector.java @@ -96,7 +96,8 @@ public abstract class AbstractNetworkConnector extends AbstractConnector impleme @Override public void close() { - setAccepting(false); + // Interrupting is often sufficient to close the channel + interruptAcceptors(); } @@ -106,7 +107,13 @@ public abstract class AbstractNetworkConnector extends AbstractConnector impleme close(); return super.shutdown(); } - + + @Override + protected boolean isAccepting() + { + return super.isAccepting() && isOpen(); + } + @Override public String toString() { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ServerConnector.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ServerConnector.java index c527a2ebdce..6258e171589 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ServerConnector.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ServerConnector.java @@ -344,14 +344,14 @@ public class ServerConnector extends AbstractNetworkConnector @Override public void close() { - super.close(); - ServerSocketChannel serverChannel = _acceptChannel; _acceptChannel = null; + if (serverChannel != null) { removeBean(serverChannel); + // If the interrupt did not close it, we should close it if (serverChannel.isOpen()) { try @@ -364,6 +364,7 @@ public class ServerConnector extends AbstractNetworkConnector } } } + // super.close(); _localPort = -2; } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/TypeUtilTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/TypeUtilTest.java index 836ffa009e9..d7085b9c77f 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/TypeUtilTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/TypeUtilTest.java @@ -27,7 +27,6 @@ import java.nio.file.Paths; import org.eclipse.jetty.toolchain.test.JDK; import org.junit.Assert; -import org.junit.Assume; import org.junit.Test; public class TypeUtilTest @@ -131,9 +130,7 @@ public class TypeUtilTest @Test public void testGetLocationOfClass() throws Exception { - String mavenRepoPathProperty = System.getProperty( "mavenRepoPath"); - Assume.assumeNotNull(mavenRepoPathProperty); - Path mavenRepoPath = Paths.get( mavenRepoPathProperty ); + Path mavenRepoPath = Paths.get( System.getProperty( "mavenRepoPath" ) ); String mavenRepo = mavenRepoPath.toFile().getPath().replaceAll("\\\\", "/"); // Classes from maven dependencies