From 561ea0dafb77efaa97cb04dd0db31650ad563129 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 24 Apr 2019 09:33:14 +0200 Subject: [PATCH 01/17] Issue #3550 Fixed QueuedThreadPool dump of known threads Signed-off-by: Greg Wilkins --- .../jetty/util/thread/QueuedThreadPool.java | 203 +++++++++--------- .../jetty/util/thread/TryExecutor.java | 16 +- .../util/thread/QueuedThreadPoolTest.java | 92 +++++++- 3 files changed, 212 insertions(+), 99 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 05742b0299a..d15d2c19a55 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -31,6 +31,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import org.eclipse.jetty.util.BlockingArrayQueue; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.annotation.ManagedAttribute; import org.eclipse.jetty.util.annotation.ManagedObject; import org.eclipse.jetty.util.annotation.ManagedOperation; @@ -135,7 +136,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override protected void doStart() throws Exception { - _tryExecutor = new ReservedThreadExecutor(this,_reservedThreads); + _tryExecutor = _reservedThreads==0 ? NO_TRY : new ReservedThreadExecutor(this,_reservedThreads); addBean(_tryExecutor); super.doStart(); @@ -603,7 +604,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP String knownMethod = ""; for (StackTraceElement t : trace) { - if ("idleJobPoll".equals(t.getMethodName()) && t.getClassName().endsWith("QueuedThreadPool")) + if ("idleJobPoll".equals(t.getMethodName()) && t.getClassName().endsWith("QueuedThreadPool$Runner")) { knownMethod = "IDLE "; break; @@ -636,11 +637,10 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override public void dump(Appendable out, String indent) throws IOException { - String s = thread.getId()+" "+thread.getName()+" "+thread.getState()+" "+thread.getPriority(); - if (known.length()==0) - Dumpable.dumpObjects(out, indent, s, (Object[])trace); + if (StringUtil.isBlank(known)) + Dumpable.dumpObjects(out, indent, String.format("%s %s %s %d", thread.getId(), thread.getName(), thread.getState(), thread.getPriority()), (Object[])trace); else - Dumpable.dumpObjects(out, indent, s); + Dumpable.dumpObjects(out, indent, String.format("%s %s %s %s %d", thread.getId(), thread.getName(), known, thread.getState(), thread.getPriority())); } @Override @@ -671,7 +671,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override public String toString() { - return String.format("%s[%s]@%x{%s,%d<=%d<=%d,i=%d,q=%d}[%s]", + return String.format("%s[%s]@%x{%s,%d<=%d<=%d,i=%d,r=%d,q=%d}[%s]", getClass().getSimpleName(), _name, hashCode(), @@ -680,100 +680,12 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP getThreads(), getMaxThreads(), getIdleThreads(), + getReservedThreads(), _jobs.size(), _tryExecutor); } - private Runnable _runnable = new Runnable() - { - @Override - public void run() - { - boolean idle = false; - - try - { - Runnable job = _jobs.poll(); - if (job != null && _threadsIdle.get() == 0) - startThreads(1); - - while (true) - { - if (job == null) - { - if (!idle) - { - idle = true; - _threadsIdle.incrementAndGet(); - } - - if (_idleTimeout <= 0) - job = _jobs.take(); - else - { - // maybe we should shrink? - int size = _threadsStarted.get(); - if (size > _minThreads) - { - long last = _lastShrink.get(); - long now = System.nanoTime(); - if (last == 0 || (now - last) > TimeUnit.MILLISECONDS.toNanos(_idleTimeout)) - { - if (_lastShrink.compareAndSet(last, now)) - break; - } - } - - job = _jobs.poll(_idleTimeout, TimeUnit.MILLISECONDS); - } - } - - // run job - if (job != null) - { - if (idle) - { - idle = false; - if (_threadsIdle.decrementAndGet() == 0) - startThreads(1); - } - - if (LOG.isDebugEnabled()) - LOG.debug("run {}", job); - runJob(job); - if (LOG.isDebugEnabled()) - LOG.debug("ran {}", job); - - // Clear interrupted status - Thread.interrupted(); - } - - if (!isRunning()) - break; - - job = _jobs.poll(); - } - } - catch (InterruptedException e) - { - LOG.ignore(e); - } - catch (Throwable e) - { - LOG.warn(String.format("Unexpected thread death: %s in %s", this, QueuedThreadPool.this), e); - } - finally - { - if (idle) - _threadsIdle.decrementAndGet(); - - removeThread(Thread.currentThread()); - - if (_threadsStarted.decrementAndGet() < getMinThreads()) - startThreads(1); - } - } - }; + private final Runnable _runnable = new Runner(); /** *

Runs the given job in the {@link Thread#currentThread() current thread}.

@@ -843,4 +755,101 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP } return null; } + + private static Runnable SHRINK = ()->{}; + private class Runner implements Runnable + { + @Override + public void run() + { + boolean idle = false; + + try + { + Runnable job = _jobs.poll(); + if (job != null && _threadsIdle.get() == 0) + startThreads(1); + + while (true) + { + if (job == null) + { + if (!idle) + { + idle = true; + _threadsIdle.incrementAndGet(); + } + + job = idleJobPoll(); + if (job==SHRINK) + break; + } + + // run job + if (job != null) + { + if (idle) + { + idle = false; + if (_threadsIdle.decrementAndGet() == 0) + startThreads(1); + } + + if (LOG.isDebugEnabled()) + LOG.debug("run {}", job); + runJob(job); + if (LOG.isDebugEnabled()) + LOG.debug("ran {}", job); + + // Clear interrupted status + Thread.interrupted(); + } + + if (!isRunning()) + break; + + job = _jobs.poll(); + } + } + catch (InterruptedException e) + { + LOG.ignore(e); + } + catch (Throwable e) + { + LOG.warn(String.format("Unexpected thread death: %s in %s", this, QueuedThreadPool.this), e); + } + finally + { + if (idle) + _threadsIdle.decrementAndGet(); + + removeThread(Thread.currentThread()); + + if (_threadsStarted.decrementAndGet() < getMinThreads()) + startThreads(1); + } + } + + private Runnable idleJobPoll() throws InterruptedException + { + if (_idleTimeout <= 0) + return _jobs.take(); + + // maybe we should shrink? + int size = _threadsStarted.get(); + if (size > _minThreads) + { + long last = _lastShrink.get(); + long now = System.nanoTime(); + if (last == 0 || (now - last) > TimeUnit.MILLISECONDS.toNanos(_idleTimeout)) + { + if (_lastShrink.compareAndSet(last, now)) + return SHRINK; + } + } + + return _jobs.poll(_idleTimeout, TimeUnit.MILLISECONDS); + } + } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/TryExecutor.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/TryExecutor.java index a9696fb6db9..034261a5ca4 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/TryExecutor.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/TryExecutor.java @@ -75,5 +75,19 @@ public interface TryExecutor extends Executor } } - public static final TryExecutor NO_TRY = task -> false; + TryExecutor NO_TRY = new TryExecutor() + { + @Override + public boolean tryExecute(Runnable task) + { + return false; + } + + @Override + public String toString() + { + return "NO_TRY"; + } + }; + } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java index ccf30c343ad..0199c7f0171 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java @@ -33,6 +33,7 @@ import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; +import static org.hamcrest.core.StringContains.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -368,10 +369,99 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest }); } + @Test + public void testDump() throws Exception + { + QueuedThreadPool pool = new QueuedThreadPool(4, 3); + + String dump = pool.dump(); + // TODO use hamcrest 2.0 regex matcher + assertThat(dump,containsString("STOPPED")); + assertThat(dump,containsString(",3<=0<=4,i=0,r=-1,q=0")); + assertThat(dump,containsString("[NO_TRY]")); + + pool.setReservedThreads(2); + dump = pool.dump(); + assertThat(dump,containsString("STOPPED")); + assertThat(dump,containsString(",3<=0<=4,i=0,r=2,q=0")); + assertThat(dump,containsString("[NO_TRY]")); + + pool.start(); + dump = pool.dump(); + assertThat(count(dump," - STARTED"),is(2)); + assertThat(dump,containsString(",3<=3<=4,i=3,r=2,q=0")); + assertThat(dump,containsString("[ReservedThreadExecutor@")); + assertThat(count(dump," IDLE "),is(3)); + assertThat(count(dump," RESERVED "),is(0)); + + + CountDownLatch started = new CountDownLatch(1); + CountDownLatch waiting = new CountDownLatch(1); + pool.execute(()-> + { + try + { + started.countDown(); + waiting.await(); + } + catch (InterruptedException e) + { + e.printStackTrace(); + } + }); + started.await(); + + dump = pool.dump(); + assertThat(count(dump," - STARTED"),is(2)); + assertThat(dump,containsString(",3<=3<=4,i=2,r=2,q=0")); + assertThat(dump,containsString("[ReservedThreadExecutor@")); + assertThat(count(dump," IDLE "),is(2)); + assertThat(count(dump," WAITING "),is(1)); + assertThat(count(dump," RESERVED "),is(0)); + assertThat(count(dump,"QueuedThreadPoolTest.lambda$testDump$"),is(0)); + + pool.setDetailedDump(true); + dump = pool.dump(); + assertThat(count(dump," - STARTED"),is(2)); + assertThat(dump,containsString(",3<=3<=4,i=2,r=2,q=0")); + assertThat(dump,containsString("s=0/2")); + assertThat(dump,containsString("[ReservedThreadExecutor@")); + assertThat(count(dump," IDLE "),is(2)); + assertThat(count(dump," WAITING "),is(1)); + assertThat(count(dump," RESERVED "),is(0)); + assertThat(count(dump,"QueuedThreadPoolTest.lambda$testDump$"),is(1)); + + assertFalse(pool.tryExecute(()->{})); + while(pool.getIdleThreads()==2) + Thread.sleep(10); + + dump = pool.dump(); + assertThat(count(dump," - STARTED"),is(2)); + assertThat(dump,containsString(",3<=3<=4,i=1,r=2,q=0")); + assertThat(dump,containsString("s=1/2")); + assertThat(dump,containsString("[ReservedThreadExecutor@")); + assertThat(count(dump," IDLE "),is(1)); + assertThat(count(dump," WAITING "),is(1)); + assertThat(count(dump," RESERVED "),is(1)); + assertThat(count(dump,"QueuedThreadPoolTest.lambda$testDump$"),is(1)); + } + + private int count(String s, String p) + { + int c = 0; + int i = s.indexOf(p); + while (i>=0) + { + c++; + i = s.indexOf(p, i+1); + } + return c; + } + @Override protected SizedThreadPool newPool(int max) { return new QueuedThreadPool(max); } - + } From f69de3372ec97a6c4547a48696f92b8d08a369ef Mon Sep 17 00:00:00 2001 From: lachan-roberts Date: Wed, 24 Apr 2019 18:46:20 +1000 Subject: [PATCH 02/17] Issue #3550 - start new thread on execute if there are no idle threads previously if there were no idle threads, QueuedThreadPool.execute() would just queue the job and not start a new thread to run it Signed-off-by: lachan-roberts --- .../jetty/util/thread/QueuedThreadPool.java | 7 ++-- .../util/thread/QueuedThreadPoolTest.java | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index d15d2c19a55..9cacfca2f31 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -474,7 +474,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP else { // Make sure there is at least one thread executing the job. - if (getThreads() == 0) + if (getQueueSize() > 0 && getIdleThreads() == 0) startThreads(1); } } @@ -781,8 +781,11 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP } job = idleJobPoll(); - if (job==SHRINK) + if (job == SHRINK) + { + LOG.warn("shrinking {}", this); break; + } } // run job diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java index 0199c7f0171..45711f94acf 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java @@ -167,6 +167,39 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest waitForIdle(tp,2); } + @Test + public void testExecuteNoIdleThreads() throws Exception + { + QueuedThreadPool tp= new QueuedThreadPool(); + tp.setDetailedDump(true); + tp.setMinThreads(3); + tp.setMaxThreads(10); + tp.setIdleTimeout(500); + + tp.start(); + + RunningJob job1 = new RunningJob(); + tp.execute(job1); + + RunningJob job2 = new RunningJob(); + tp.execute(job2); + + RunningJob job3 = new RunningJob(); + tp.execute(job3); + + // make sure these jobs have started running + assertTrue(job1._run.await(5, TimeUnit.SECONDS)); + assertTrue(job2._run.await(5, TimeUnit.SECONDS)); + assertTrue(job3._run.await(5, TimeUnit.SECONDS)); + + waitForThreads(tp, 4); + waitForThreads(tp, 3); + + RunningJob job4 = new RunningJob(); + tp.execute(job4); + assertTrue(job4._run.await(5, TimeUnit.SECONDS)); + } + @Test public void testLifeCycleStop() throws Exception { From c4d51b09df7a181efcbe6a130f701a95fecea37d Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 24 Apr 2019 11:42:23 +0200 Subject: [PATCH 03/17] Issue #3550 Ensure that new threads are started if a thread exits due to failure. Signed-off-by: Greg Wilkins --- .../jetty/util/thread/QueuedThreadPool.java | 7 +- .../util/thread/QueuedThreadPoolTest.java | 98 ++++++++++++++++++- 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 9cacfca2f31..131bcd14f09 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -783,7 +783,8 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP job = idleJobPoll(); if (job == SHRINK) { - LOG.warn("shrinking {}", this); + if (LOG.isDebugEnabled()) + LOG.debug("shrinking {}", this); break; } } @@ -829,7 +830,9 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP removeThread(Thread.currentThread()); - if (_threadsStarted.decrementAndGet() < getMinThreads()) + int threads = _threadsStarted.decrementAndGet(); + // We should start a new thread if threads are now less than min threads or we have queued jobs + if (threads < getMinThreads() || getQueueSize()>0) startThreads(1); } } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java index 45711f94acf..06e28fc4ee9 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java @@ -50,6 +50,16 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest private final CountDownLatch _run = new CountDownLatch(1); private final CountDownLatch _stopping = new CountDownLatch(1); private final CountDownLatch _stopped = new CountDownLatch(1); + private final boolean _fail; + RunningJob() + { + this(false); + } + + public RunningJob(boolean fail) + { + _fail = fail; + } @Override public void run() @@ -58,6 +68,12 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest { _run.countDown(); _stopping.await(); + if (_fail) + throw new IllegalStateException("Testing!"); + } + catch(IllegalStateException e) + { + throw e; } catch(Exception e) { @@ -167,6 +183,86 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest waitForIdle(tp,2); } + @Test + public void testThreadPoolFailingJobs() throws Exception + { + try (StacklessLogging stackless = new StacklessLogging(QueuedThreadPool.class)) + { + QueuedThreadPool tp = new QueuedThreadPool(); + tp.setMinThreads(2); + tp.setMaxThreads(4); + tp.setIdleTimeout(900); + tp.setThreadsPriority(Thread.NORM_PRIORITY - 1); + + tp.start(); + + // min threads started + waitForThreads(tp, 2); + waitForIdle(tp, 2); + + // Doesn't shrink less than 1 + Thread.sleep(1100); + waitForThreads(tp, 2); + waitForIdle(tp, 2); + + // Run job0 + RunningJob job0 = new RunningJob(true); + tp.execute(job0); + assertTrue(job0._run.await(10, TimeUnit.SECONDS)); + waitForIdle(tp, 1); + + // Run job1 + RunningJob job1 = new RunningJob(true); + tp.execute(job1); + assertTrue(job1._run.await(10, TimeUnit.SECONDS)); + waitForThreads(tp, 3); + waitForIdle(tp, 1); + + // Run job2 + RunningJob job2 = new RunningJob(true); + tp.execute(job2); + assertTrue(job2._run.await(10, TimeUnit.SECONDS)); + waitForThreads(tp, 4); + waitForIdle(tp, 1); + + // Run job3 + RunningJob job3 = new RunningJob(true); + tp.execute(job3); + assertTrue(job3._run.await(10, TimeUnit.SECONDS)); + waitForThreads(tp, 4); + assertThat(tp.getIdleThreads(), is(0)); + Thread.sleep(100); + assertThat(tp.getIdleThreads(), is(0)); + + // Run job4. will be queued + RunningJob job4 = new RunningJob(true); + tp.execute(job4); + assertFalse(job4._run.await(1, TimeUnit.SECONDS)); + + // finish job 0 + job0._stopping.countDown(); + assertTrue(job0._stopped.await(10, TimeUnit.SECONDS)); + + // job4 should now run + assertTrue(job4._run.await(10, TimeUnit.SECONDS)); + waitForThreads(tp, 4); + waitForIdle(tp, 0); + + // finish job 1,2,3,4 + job1._stopping.countDown(); + job2._stopping.countDown(); + job3._stopping.countDown(); + job4._stopping.countDown(); + assertTrue(job1._stopped.await(10, TimeUnit.SECONDS)); + assertTrue(job2._stopped.await(10, TimeUnit.SECONDS)); + assertTrue(job3._stopped.await(10, TimeUnit.SECONDS)); + assertTrue(job4._stopped.await(10, TimeUnit.SECONDS)); + + waitForThreads(tp, 2); + waitForIdle(tp, 2); + } + } + @Test public void testExecuteNoIdleThreads() throws Exception { @@ -420,6 +516,7 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest assertThat(dump,containsString("[NO_TRY]")); pool.start(); + waitForIdle(pool,3); dump = pool.dump(); assertThat(count(dump," - STARTED"),is(2)); assertThat(dump,containsString(",3<=3<=4,i=3,r=2,q=0")); @@ -427,7 +524,6 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest assertThat(count(dump," IDLE "),is(3)); assertThat(count(dump," RESERVED "),is(0)); - CountDownLatch started = new CountDownLatch(1); CountDownLatch waiting = new CountDownLatch(1); pool.execute(()-> From 787380a91ecf54331ba7d87c6fa369e0fa3f33a4 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 24 Apr 2019 22:20:32 +0200 Subject: [PATCH 04/17] Improve #3550 fix with a single Atomic Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/util/AtomicWords.java | 182 ++++++++++++++++++ .../jetty/util/thread/QueuedThreadPool.java | 176 +++++++++-------- .../util/thread/QueuedThreadPoolTest.java | 174 +++++++++++------ 3 files changed, 378 insertions(+), 154 deletions(-) create mode 100644 jetty-util/src/main/java/org/eclipse/jetty/util/AtomicWords.java diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicWords.java b/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicWords.java new file mode 100644 index 00000000000..5a3e0c638f7 --- /dev/null +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicWords.java @@ -0,0 +1,182 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.util; + +import java.util.concurrent.atomic.AtomicLong; + +/** + * An AtomicLong with additional methods to treat it as two hi/lo integers. + */ +public class AtomicWords extends AtomicLong +{ + /** + * Sets the hi and lo values. + * + * @param w0 the 0th word + * @param w1 the 1st word + * @param w2 the 2nd word + * @param w3 the 3rd word + */ + public void set(int w0, int w1, int w2, int w3) + { + set(encode(w0, w1, w2, w3)); + } + + /** + * Atomically sets the word values to the given updated values only if + * the current encoded value is as expected. + * + * @param expectEncoded the expected encoded value + * @param w0 the 0th word + * @param w1 the 1st word + * @param w2 the 2nd word + * @param w3 the 3rd word + * @return {@code true} if successful. + */ + public boolean compareAndSet(long expectEncoded, int w0, int w1, int w2, int w3) + { + return compareAndSet(expectEncoded,encode(w0, w1, w2, w3)); + } + + /** + * Atomically adds the given deltas to the current hi and lo values. + * + * @param delta0 the delta to apply to the 0th word value + * @param delta1 the delta to apply to the 1st word value + * @param delta2 the delta to apply to the 2nd word value + * @param delta3 the delta to apply to the 3rd word value + */ + public void add(int delta0, int delta1, int delta2, int delta3) + { + while(true) + { + long encoded = get(); + long update = encode(getWord0(encoded)+delta0, + getWord1(encoded)+delta1, + getWord2(encoded)+delta2, + getWord3(encoded)+delta3); + if (compareAndSet(encoded,update)) + return; + } + } + + + /** + * Gets word 0 value + * + * @return the 16 bit value as an int + */ + public int getWord0() + { + return getWord0(get()); + } + + + /** + * Gets word 1 value + * + * @return the 16 bit value as an int + */ + public int getWord1() + { + return getWord1(get()); + } + + /** + * Gets word 2 value + * + * @return the 16 bit value as an int + */ + public int getWord2() + { + return getWord2(get()); + } + + /** + * Gets word 3 value + * + * @return the 16 bit value as an int + */ + public int getWord3() + { + return getWord3(get()); + } + + /** + * Gets word 0 value from the given encoded value. + * + * @param encoded the encoded value + * @return the 16 bit value as an int + */ + public static int getWord0(long encoded) + { + return (int) ((encoded>>48)&0xFFFFL); + } + + /** + * Gets word 0 value from the given encoded value. + * + * @param encoded the encoded value + * @return the 16 bit value as an int + */ + public static int getWord1(long encoded) + { + return (int) ((encoded>>32)&0xFFFFL); + } + + /** + * Gets word 0 value from the given encoded value. + * + * @param encoded the encoded value + * @return the 16 bit value as an int + */ + public static int getWord2(long encoded) + { + return (int) ((encoded>>16)&0xFFFFL); + } + + /** + * Gets word 0 value from the given encoded value. + * + * @param encoded the encoded value + * @return the 16 bit value as an int + */ + public static int getWord3(long encoded) + { + return (int) (encoded&0xFFFFL); + } + + /** + * Encodes 4 16 bit words values into a long. + * + * @param w0 the 0th word + * @param w1 the 1st word + * @param w2 the 2nd word + * @param w3 the 3rd word + * @return the encoded value + */ + public static long encode(int w0, int w1, int w2, int w3) + { + long wl0 = ((long)w0)&0xFFFFL; + long wl1 = ((long)w1)&0xFFFFL; + long wl2 = ((long)w2)&0xFFFFL; + long wl3 = ((long)w3)&0xFFFFL; + return (wl0<<48)+(wl1<<32)+(wl2<<16)+wl3; + } +} diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 131bcd14f09..a2a5653a4b4 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -27,9 +27,9 @@ import java.util.concurrent.BlockingQueue; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import org.eclipse.jetty.util.AtomicWords; import org.eclipse.jetty.util.BlockingArrayQueue; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.annotation.ManagedAttribute; @@ -49,8 +49,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP { private static final Logger LOG = Log.getLogger(QueuedThreadPool.class); - private final AtomicInteger _threadsStarted = new AtomicInteger(); - private final AtomicInteger _threadsIdle = new AtomicInteger(); + private final AtomicWords _counts = new AtomicWords(); private final AtomicLong _lastShrink = new AtomicLong(); private final Set _threads = ConcurrentHashMap.newKeySet(); private final Object _joinLock = new Object(); @@ -140,9 +139,8 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP addBean(_tryExecutor); super.doStart(); - _threadsStarted.set(0); - - startThreads(_minThreads); + _counts.set(0,0,0,0); + ensureThreads(); } @Override @@ -165,42 +163,25 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP // Fill job Q with noop jobs to wakeup idle Runnable noop = () -> {}; - for (int i = _threadsStarted.get(); i-- > 0; ) + for (int i = _counts.getWord0(); i-- > 0; ) jobs.offer(noop); // try to let jobs complete naturally for half our stop time - long stopby = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeout) / 2; - for (Thread thread : _threads) - { - long canwait = TimeUnit.NANOSECONDS.toMillis(stopby - System.nanoTime()); - if (LOG.isDebugEnabled()) - LOG.debug("Waiting for {} for {}", thread, canwait); - if (canwait > 0) - thread.join(canwait); - } + joinThreads(System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeout) / 2); // If we still have threads running, get a bit more aggressive // interrupt remaining threads - if (_threadsStarted.get() > 0) - for (Thread thread : _threads) - { - if (LOG.isDebugEnabled()) - LOG.debug("Interrupting {}", thread); - thread.interrupt(); - } - - // wait again for the other half of our stop time - stopby = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeout) / 2; for (Thread thread : _threads) { - long canwait = TimeUnit.NANOSECONDS.toMillis(stopby - System.nanoTime()); if (LOG.isDebugEnabled()) - LOG.debug("Waiting for {} for {}", thread, canwait); - if (canwait > 0) - thread.join(canwait); + LOG.debug("Interrupting {}", thread); + thread.interrupt(); } + // wait again for the other half of our stop time + joinThreads(System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeout) / 2); + Thread.yield(); int size = _threads.size(); if (size > 0) @@ -254,6 +235,18 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP } } + private void joinThreads(long stopByNanos) throws InterruptedException + { + for (Thread thread : _threads) + { + long canWait = TimeUnit.NANOSECONDS.toMillis(stopByNanos - System.nanoTime()); + if (LOG.isDebugEnabled()) + LOG.debug("Waiting for {} for {}", thread, canWait); + if (canWait > 0) + thread.join(canWait); + } + } + /** * Thread Pool should use Daemon Threading. * @@ -308,9 +301,8 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP if (_minThreads > _maxThreads) _maxThreads = _minThreads; - int threads = _threadsStarted.get(); - if (isStarted() && threads < _minThreads) - startThreads(_minThreads - threads); + if (isStarted()) + ensureThreads(); } /** @@ -471,12 +463,9 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP LOG.warn("{} rejected {}", this, job); throw new RejectedExecutionException(job.toString()); } - else - { - // Make sure there is at least one thread executing the job. - if (getQueueSize() > 0 && getIdleThreads() == 0) - startThreads(1); - } + + // Make sure there is at least one thread executing the job. + ensureThreads(); } @Override @@ -509,7 +498,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @ManagedAttribute("number of threads in the pool") public int getThreads() { - return _threadsStarted.get(); + return _counts.getWord0(); } /** @@ -519,7 +508,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @ManagedAttribute("number of idle threads in the pool") public int getIdleThreads() { - return _threadsIdle.get(); + return _counts.getWord3(); } /** @@ -549,20 +538,29 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP return getMaxThreads() - getThreads() + getIdleThreads() - getQueueSize() <= getLowThreadsThreshold(); } - private boolean startThreads(int threadsToStart) + private void ensureThreads() { - while (threadsToStart > 0 && isRunning()) + while (isRunning()) { - int threads = _threadsStarted.get(); - if (threads >= _maxThreads) - return false; + long count = _counts.get(); + int threads = AtomicWords.getWord0(count); + int starting = AtomicWords.getWord1(count); + int idle = AtomicWords.getWord3(count); + int queue = getQueueSize(); - if (!_threadsStarted.compareAndSet(threads, threads + 1)) + if (threads >= _maxThreads) + break; + if (threads >= _minThreads && (starting + idle) >= queue) + break; + if (!_counts.compareAndSet(count, threads + 1, starting + 1, 0, idle)) continue; boolean started = false; try { + if (LOG.isDebugEnabled()) + LOG.debug("Starting thread {}",this); + Thread thread = newThread(_runnable); thread.setDaemon(isDaemon()); thread.setPriority(getThreadsPriority()); @@ -573,15 +571,13 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP _lastShrink.set(System.nanoTime()); thread.start(); started = true; - --threadsToStart; } finally { if (!started) - _threadsStarted.decrementAndGet(); + _counts.add(-1,-1,0,0); } } - return true; } protected Thread newThread(Runnable runnable) @@ -671,17 +667,24 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override public String toString() { - return String.format("%s[%s]@%x{%s,%d<=%d<=%d,i=%d,r=%d,q=%d}[%s]", + long count = _counts.get(); + int threads = AtomicWords.getWord0(count); + int starting = AtomicWords.getWord1(count); + int idle = AtomicWords.getWord3(count); + int queue = getQueueSize(); + + return String.format("%s[%s]@%x{%s,%d<=%d<=%d,s=%d,i=%d,r=%d,q=%d}[%s]", getClass().getSimpleName(), _name, hashCode(), getState(), getMinThreads(), - getThreads(), + threads, getMaxThreads(), - getIdleThreads(), + starting, + idle, getReservedThreads(), - _jobs.size(), + queue, _tryExecutor); } @@ -756,19 +759,19 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP return null; } - private static Runnable SHRINK = ()->{}; private class Runner implements Runnable { @Override public void run() { boolean idle = false; + Runnable job = null; try { - Runnable job = _jobs.poll(); - if (job != null && _threadsIdle.get() == 0) - startThreads(1); + job = _jobs.poll(); + idle = job==null; + _counts.add(0,-1,0,idle?1:0); while (true) { @@ -777,15 +780,29 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP if (!idle) { idle = true; - _threadsIdle.incrementAndGet(); + _counts.add(0,0,0,1); } job = idleJobPoll(); - if (job == SHRINK) + + if (job == null) { - if (LOG.isDebugEnabled()) - LOG.debug("shrinking {}", this); - break; + // maybe we should shrink? + int size = getThreads(); + if (size > _minThreads) + { + long last = _lastShrink.get(); + long now = System.nanoTime(); + if (last == 0 || (now - last) > TimeUnit.MILLISECONDS.toNanos(_idleTimeout)) + { + if (_lastShrink.compareAndSet(last, now)) + { + if (LOG.isDebugEnabled()) + LOG.debug("shrinking {}", QueuedThreadPool.this); + break; + } + } + } } } @@ -795,15 +812,14 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP if (idle) { idle = false; - if (_threadsIdle.decrementAndGet() == 0) - startThreads(1); + _counts.add(0,0,0,-1); } if (LOG.isDebugEnabled()) - LOG.debug("run {}", job); + LOG.debug("run {} in {}", job, QueuedThreadPool.this); runJob(job); if (LOG.isDebugEnabled()) - LOG.debug("ran {}", job); + LOG.debug("ran {} in {}", job, QueuedThreadPool.this); // Clear interrupted status Thread.interrupted(); @@ -821,19 +837,13 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP } catch (Throwable e) { - LOG.warn(String.format("Unexpected thread death: %s in %s", this, QueuedThreadPool.this), e); + LOG.warn(String.format("Unexpected thread death: %s in %s", job, QueuedThreadPool.this), e); } finally { - if (idle) - _threadsIdle.decrementAndGet(); - + _counts.add(-1,0,0,idle?-1:0); removeThread(Thread.currentThread()); - - int threads = _threadsStarted.decrementAndGet(); - // We should start a new thread if threads are now less than min threads or we have queued jobs - if (threads < getMinThreads() || getQueueSize()>0) - startThreads(1); + ensureThreads(); } } @@ -841,20 +851,6 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP { if (_idleTimeout <= 0) return _jobs.take(); - - // maybe we should shrink? - int size = _threadsStarted.get(); - if (size > _minThreads) - { - long last = _lastShrink.get(); - long now = System.nanoTime(); - if (last == 0 || (now - last) > TimeUnit.MILLISECONDS.toNanos(_idleTimeout)) - { - if (_lastShrink.compareAndSet(last, now)) - return SHRINK; - } - } - return _jobs.poll(_idleTimeout, TimeUnit.MILLISECONDS); } } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java index 06e28fc4ee9..1f00ac25f9c 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java @@ -28,6 +28,7 @@ import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.log.StacklessLogging; import org.eclipse.jetty.util.thread.ThreadPool.SizedThreadPool; +import org.hamcrest.Matchers; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; @@ -50,14 +51,26 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest private final CountDownLatch _run = new CountDownLatch(1); private final CountDownLatch _stopping = new CountDownLatch(1); private final CountDownLatch _stopped = new CountDownLatch(1); + private final String _name; private final boolean _fail; RunningJob() { - this(false); + this(null, false); + } + + public RunningJob(String name) + { + this(name, false); } public RunningJob(boolean fail) { + this(null, fail); + } + + public RunningJob(String name, boolean fail) + { + _name = name; _fail = fail; } @@ -93,6 +106,14 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest if (!_stopped.await(10,TimeUnit.SECONDS)) throw new IllegalStateException(); } + + @Override + public String toString() + { + if (_name==null) + return super.toString(); + return String.format("%s@%x",_name,hashCode()); + } } private class CloseableJob extends RunningJob implements Closeable @@ -121,42 +142,43 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest waitForThreads(tp,2); waitForIdle(tp,2); - // Doesn't shrink less than 1 - Thread.sleep(1100); + // Doesn't shrink to less than min threads + Thread.sleep(3*tp.getIdleTimeout()/2); waitForThreads(tp,2); waitForIdle(tp,2); // Run job0 - RunningJob job0=new RunningJob(); + RunningJob job0=new RunningJob("JOB0"); tp.execute(job0); assertTrue(job0._run.await(10,TimeUnit.SECONDS)); waitForIdle(tp,1); // Run job1 - RunningJob job1=new RunningJob(); + RunningJob job1=new RunningJob("JOB1"); tp.execute(job1); assertTrue(job1._run.await(10,TimeUnit.SECONDS)); - waitForThreads(tp,3); - waitForIdle(tp,1); + waitForThreads(tp,2); + waitForIdle(tp,0); // Run job2 - RunningJob job2=new RunningJob(); + RunningJob job2=new RunningJob("JOB2"); tp.execute(job2); assertTrue(job2._run.await(10,TimeUnit.SECONDS)); - waitForThreads(tp,4); - waitForIdle(tp,1); + waitForThreads(tp,3); + waitForIdle(tp,0); // Run job3 - RunningJob job3=new RunningJob(); + RunningJob job3=new RunningJob("JOB3"); tp.execute(job3); assertTrue(job3._run.await(10,TimeUnit.SECONDS)); waitForThreads(tp,4); + waitForIdle(tp,0); assertThat(tp.getIdleThreads(),is(0)); Thread.sleep(100); assertThat(tp.getIdleThreads(),is(0)); // Run job4. will be queued - RunningJob job4=new RunningJob(); + RunningJob job4=new RunningJob("JOB4"); tp.execute(job4); assertFalse(job4._run.await(1,TimeUnit.SECONDS)); @@ -166,21 +188,32 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest // job4 should now run assertTrue(job4._run.await(10,TimeUnit.SECONDS)); - waitForThreads(tp,4); - waitForIdle(tp,0); - - // finish job 1,2,3,4 + assertThat(tp.getThreads(),is(4)); + assertThat(tp.getIdleThreads(),is(0)); + + // finish job 1 job1._stopping.countDown(); + assertTrue(job1._stopped.await(10,TimeUnit.SECONDS)); + waitForIdle(tp,1); + assertThat(tp.getThreads(),is(4)); + + // finish job 2,3,4 job2._stopping.countDown(); job3._stopping.countDown(); job4._stopping.countDown(); - assertTrue(job1._stopped.await(10,TimeUnit.SECONDS)); assertTrue(job2._stopped.await(10,TimeUnit.SECONDS)); assertTrue(job3._stopped.await(10,TimeUnit.SECONDS)); assertTrue(job4._stopped.await(10,TimeUnit.SECONDS)); - waitForThreads(tp,2); - waitForIdle(tp,2); + waitForIdle(tp,4); + assertThat(tp.getThreads(),is(4)); + + long duration = System.nanoTime(); + waitForThreads(tp,3); + assertThat(tp.getIdleThreads(),is(3)); + duration = System.nanoTime() - duration; + assertThat(TimeUnit.NANOSECONDS.toMillis(duration), Matchers.greaterThan(tp.getIdleTimeout()/2L)); + assertThat(TimeUnit.NANOSECONDS.toMillis(duration), Matchers.lessThan(tp.getIdleTimeout()*2L)); } @Test @@ -188,78 +221,83 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest { try (StacklessLogging stackless = new StacklessLogging(QueuedThreadPool.class)) { - QueuedThreadPool tp = new QueuedThreadPool(); + QueuedThreadPool tp= new QueuedThreadPool(); tp.setMinThreads(2); tp.setMaxThreads(4); tp.setIdleTimeout(900); - tp.setThreadsPriority(Thread.NORM_PRIORITY - 1); + tp.setThreadsPriority(Thread.NORM_PRIORITY-1); tp.start(); // min threads started - waitForThreads(tp, 2); - waitForIdle(tp, 2); + waitForThreads(tp,2); + waitForIdle(tp,2); - // Doesn't shrink less than 1 - Thread.sleep(1100); - waitForThreads(tp, 2); - waitForIdle(tp, 2); + // Doesn't shrink to less than min threads + Thread.sleep(3*tp.getIdleTimeout()/2); + waitForThreads(tp,2); + waitForIdle(tp,2); // Run job0 - RunningJob job0 = new RunningJob(true); + RunningJob job0=new RunningJob("JOB0", true); tp.execute(job0); - assertTrue(job0._run.await(10, TimeUnit.SECONDS)); - waitForIdle(tp, 1); + assertTrue(job0._run.await(10,TimeUnit.SECONDS)); + waitForIdle(tp,1); // Run job1 - RunningJob job1 = new RunningJob(true); + RunningJob job1=new RunningJob("JOB1", true); tp.execute(job1); - assertTrue(job1._run.await(10, TimeUnit.SECONDS)); - waitForThreads(tp, 3); - waitForIdle(tp, 1); + assertTrue(job1._run.await(10,TimeUnit.SECONDS)); + waitForThreads(tp,2); + waitForIdle(tp,0); // Run job2 - RunningJob job2 = new RunningJob(true); + RunningJob job2=new RunningJob("JOB2", true); tp.execute(job2); - assertTrue(job2._run.await(10, TimeUnit.SECONDS)); - waitForThreads(tp, 4); - waitForIdle(tp, 1); + assertTrue(job2._run.await(10,TimeUnit.SECONDS)); + waitForThreads(tp,3); + waitForIdle(tp,0); // Run job3 - RunningJob job3 = new RunningJob(true); + RunningJob job3=new RunningJob("JOB3", true); tp.execute(job3); - assertTrue(job3._run.await(10, TimeUnit.SECONDS)); - waitForThreads(tp, 4); - assertThat(tp.getIdleThreads(), is(0)); + assertTrue(job3._run.await(10,TimeUnit.SECONDS)); + waitForThreads(tp,4); + waitForIdle(tp,0); + assertThat(tp.getIdleThreads(),is(0)); Thread.sleep(100); - assertThat(tp.getIdleThreads(), is(0)); + assertThat(tp.getIdleThreads(),is(0)); // Run job4. will be queued - RunningJob job4 = new RunningJob(true); + RunningJob job4=new RunningJob("JOB4", true); tp.execute(job4); - assertFalse(job4._run.await(1, TimeUnit.SECONDS)); + assertFalse(job4._run.await(1,TimeUnit.SECONDS)); // finish job 0 job0._stopping.countDown(); - assertTrue(job0._stopped.await(10, TimeUnit.SECONDS)); + assertTrue(job0._stopped.await(10,TimeUnit.SECONDS)); // job4 should now run - assertTrue(job4._run.await(10, TimeUnit.SECONDS)); - waitForThreads(tp, 4); - waitForIdle(tp, 0); + assertTrue(job4._run.await(10,TimeUnit.SECONDS)); + assertThat(tp.getThreads(),is(4)); + assertThat(tp.getIdleThreads(),is(0)); - // finish job 1,2,3,4 + // finish job 1 job1._stopping.countDown(); + assertTrue(job1._stopped.await(10,TimeUnit.SECONDS)); + waitForThreads(tp,3); + assertThat(tp.getIdleThreads(),is(0)); + + // finish job 2,3,4 job2._stopping.countDown(); job3._stopping.countDown(); job4._stopping.countDown(); - assertTrue(job1._stopped.await(10, TimeUnit.SECONDS)); - assertTrue(job2._stopped.await(10, TimeUnit.SECONDS)); - assertTrue(job3._stopped.await(10, TimeUnit.SECONDS)); - assertTrue(job4._stopped.await(10, TimeUnit.SECONDS)); + assertTrue(job2._stopped.await(10,TimeUnit.SECONDS)); + assertTrue(job3._stopped.await(10,TimeUnit.SECONDS)); + assertTrue(job4._stopped.await(10,TimeUnit.SECONDS)); - waitForThreads(tp, 2); - waitForIdle(tp, 2); + waitForIdle(tp,2); + assertThat(tp.getThreads(),is(2)); } } @@ -268,7 +306,7 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest { QueuedThreadPool tp= new QueuedThreadPool(); tp.setDetailedDump(true); - tp.setMinThreads(3); + tp.setMinThreads(1); tp.setMaxThreads(10); tp.setIdleTimeout(500); @@ -288,8 +326,16 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest assertTrue(job2._run.await(5, TimeUnit.SECONDS)); assertTrue(job3._run.await(5, TimeUnit.SECONDS)); - waitForThreads(tp, 4); waitForThreads(tp, 3); + assertThat(tp.getIdleThreads(),is(0)); + + job1._stopping.countDown(); + assertTrue(job1._stopped.await(10,TimeUnit.SECONDS)); + waitForIdle(tp, 1); + assertThat(tp.getThreads(),is(3)); + + waitForIdle(tp, 0); + assertThat(tp.getThreads(),is(2)); RunningJob job4 = new RunningJob(); tp.execute(job4); @@ -506,20 +552,20 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest String dump = pool.dump(); // TODO use hamcrest 2.0 regex matcher assertThat(dump,containsString("STOPPED")); - assertThat(dump,containsString(",3<=0<=4,i=0,r=-1,q=0")); + assertThat(dump,containsString(",3<=0<=4,s=0,i=0,r=-1,q=0")); assertThat(dump,containsString("[NO_TRY]")); pool.setReservedThreads(2); dump = pool.dump(); assertThat(dump,containsString("STOPPED")); - assertThat(dump,containsString(",3<=0<=4,i=0,r=2,q=0")); + assertThat(dump,containsString(",3<=0<=4,s=0,i=0,r=2,q=0")); assertThat(dump,containsString("[NO_TRY]")); pool.start(); waitForIdle(pool,3); dump = pool.dump(); assertThat(count(dump," - STARTED"),is(2)); - assertThat(dump,containsString(",3<=3<=4,i=3,r=2,q=0")); + assertThat(dump,containsString(",3<=3<=4,s=0,i=3,r=2,q=0")); assertThat(dump,containsString("[ReservedThreadExecutor@")); assertThat(count(dump," IDLE "),is(3)); assertThat(count(dump," RESERVED "),is(0)); @@ -542,7 +588,7 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest dump = pool.dump(); assertThat(count(dump," - STARTED"),is(2)); - assertThat(dump,containsString(",3<=3<=4,i=2,r=2,q=0")); + assertThat(dump,containsString(",3<=3<=4,s=0,i=2,r=2,q=0")); assertThat(dump,containsString("[ReservedThreadExecutor@")); assertThat(count(dump," IDLE "),is(2)); assertThat(count(dump," WAITING "),is(1)); @@ -552,7 +598,7 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest pool.setDetailedDump(true); dump = pool.dump(); assertThat(count(dump," - STARTED"),is(2)); - assertThat(dump,containsString(",3<=3<=4,i=2,r=2,q=0")); + assertThat(dump,containsString(",3<=3<=4,s=0,i=2,r=2,q=0")); assertThat(dump,containsString("s=0/2")); assertThat(dump,containsString("[ReservedThreadExecutor@")); assertThat(count(dump," IDLE "),is(2)); @@ -566,7 +612,7 @@ public class QueuedThreadPoolTest extends AbstractThreadPoolTest dump = pool.dump(); assertThat(count(dump," - STARTED"),is(2)); - assertThat(dump,containsString(",3<=3<=4,i=1,r=2,q=0")); + assertThat(dump,containsString(",3<=3<=4,s=0,i=1,r=2,q=0")); assertThat(dump,containsString("s=1/2")); assertThat(dump,containsString("[ReservedThreadExecutor@")); assertThat(count(dump," IDLE "),is(1)); From c66957867b3c88c0abaf8c5395ad983237ac44fa Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 25 Apr 2019 06:13:30 +0200 Subject: [PATCH 05/17] Improve #3550 cleanup after review Signed-off-by: Greg Wilkins --- .../java/org/eclipse/jetty/util/thread/QueuedThreadPool.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 131bcd14f09..171aa8e0692 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -604,7 +604,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP String knownMethod = ""; for (StackTraceElement t : trace) { - if ("idleJobPoll".equals(t.getMethodName()) && t.getClassName().endsWith("QueuedThreadPool$Runner")) + if ("idleJobPoll".equals(t.getMethodName()) && t.getClassName().equals(Runner.class.getName())) { knownMethod = "IDLE "; break; From 3fd486737e6b25f7ffc276449ec9cf229bd2a784 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 25 Apr 2019 12:50:05 +0200 Subject: [PATCH 06/17] Improve #3550 Use a TriInteger for larger range of threads Signed-off-by: Greg Wilkins --- ...AtomicWords.java => AtomicTriInteger.java} | 78 ++++++------- .../jetty/util/thread/QueuedThreadPool.java | 32 +++--- .../jetty/util/AtomicTriIntegerTest.java | 103 ++++++++++++++++++ 3 files changed, 154 insertions(+), 59 deletions(-) rename jetty-util/src/main/java/org/eclipse/jetty/util/{AtomicWords.java => AtomicTriInteger.java} (70%) create mode 100644 jetty-util/src/test/java/org/eclipse/jetty/util/AtomicTriIntegerTest.java diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicWords.java b/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicTriInteger.java similarity index 70% rename from jetty-util/src/main/java/org/eclipse/jetty/util/AtomicWords.java rename to jetty-util/src/main/java/org/eclipse/jetty/util/AtomicTriInteger.java index 0a01a19b983..e01c96f3cc8 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicWords.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicTriInteger.java @@ -21,21 +21,22 @@ package org.eclipse.jetty.util; import java.util.concurrent.atomic.AtomicLong; /** - * An AtomicLong with additional methods to treat it as four 16 bit words. + * An AtomicLong with additional methods to treat it as three 21 bit unsigned words. */ -public class AtomicWords extends AtomicLong +public class AtomicTriInteger extends AtomicLong { + public static int MAX_VALUE = 0x1FFFFF; + public static int MIN_VALUE = 0; /** * Sets the hi and lo values. * * @param w0 the 0th word * @param w1 the 1st word * @param w2 the 2nd word - * @param w3 the 3rd word */ - public void set(int w0, int w1, int w2, int w3) + public void set(int w0, int w1, int w2) { - set(encode(w0, w1, w2, w3)); + set(encode(w0, w1, w2)); } /** @@ -46,12 +47,11 @@ public class AtomicWords extends AtomicLong * @param w0 the 0th word * @param w1 the 1st word * @param w2 the 2nd word - * @param w3 the 3rd word * @return {@code true} if successful. */ - public boolean compareAndSet(long expectEncoded, int w0, int w1, int w2, int w3) + public boolean compareAndSet(long expectEncoded, int w0, int w1, int w2) { - return compareAndSet(expectEncoded,encode(w0, w1, w2, w3)); + return compareAndSet(expectEncoded,encode(w0, w1, w2)); } /** @@ -60,9 +60,8 @@ public class AtomicWords extends AtomicLong * @param delta0 the delta to apply to the 0th word value * @param delta1 the delta to apply to the 1st word value * @param delta2 the delta to apply to the 2nd word value - * @param delta3 the delta to apply to the 3rd word value */ - public void add(int delta0, int delta1, int delta2, int delta3) + public void add(int delta0, int delta1, int delta2) { while(true) { @@ -70,8 +69,7 @@ public class AtomicWords extends AtomicLong long update = encode( getWord0(encoded)+delta0, getWord1(encoded)+delta1, - getWord2(encoded)+delta2, - getWord3(encoded)+delta3); + getWord2(encoded)+delta2); if (compareAndSet(encoded,update)) return; } @@ -109,16 +107,6 @@ public class AtomicWords extends AtomicLong return getWord2(get()); } - /** - * Gets word 3 value - * - * @return the 16 bit value as an int - */ - public int getWord3() - { - return getWord3(get()); - } - /** * Gets word 0 value from the given encoded value. * @@ -127,7 +115,7 @@ public class AtomicWords extends AtomicLong */ public static int getWord0(long encoded) { - return (int) ((encoded>>48)&0xFFFFL); + return (int) ((encoded>>42)&0x1FFFFFL); } /** @@ -138,7 +126,7 @@ public class AtomicWords extends AtomicLong */ public static int getWord1(long encoded) { - return (int) ((encoded>>32)&0xFFFFL); + return (int) ((encoded>>21)&0x1FFFFFL); } /** @@ -149,18 +137,7 @@ public class AtomicWords extends AtomicLong */ public static int getWord2(long encoded) { - return (int) ((encoded>>16)&0xFFFFL); - } - - /** - * Gets word 0 value from the given encoded value. - * - * @param encoded the encoded value - * @return the 16 bit value as an int - */ - public static int getWord3(long encoded) - { - return (int) (encoded&0xFFFFL); + return (int) (encoded&0x1FFFFFL); } /** @@ -169,15 +146,30 @@ public class AtomicWords extends AtomicLong * @param w0 the 0th word * @param w1 the 1st word * @param w2 the 2nd word - * @param w3 the 3rd word * @return the encoded value */ - public static long encode(int w0, int w1, int w2, int w3) + public static long encode(int w0, int w1, int w2) { - long wl0 = ((long)w0)&0xFFFFL; - long wl1 = ((long)w1)&0xFFFFL; - long wl2 = ((long)w2)&0xFFFFL; - long wl3 = ((long)w3)&0xFFFFL; - return (wl0<<48)+(wl1<<32)+(wl2<<16)+wl3; + if (w0MAX_VALUE + || w1MAX_VALUE + || w2MAX_VALUE) + throw new IllegalArgumentException(String.format("Words must be 0<= word <= 0x1FFFFF: %d, %d, %d", w0, w1, w2)); + long wl0 = ((long)w0)&0x1FFFFFL; + long wl1 = ((long)w1)&0x1FFFFFL; + long wl2 = ((long)w2)&0x1FFFFFL; + return (wl0<<42)+(wl1<<21)+(wl2); + } + + @Override + public String toString() + { + long value = get(); + int w0 = getWord0(value); + int w1 = getWord1(value); + int w2 = getWord2(value); + return String.format("{%d,%d,%d}",w0,w1,w2); } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 997acf99c8a..7289a88930b 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -29,7 +29,7 @@ import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; -import org.eclipse.jetty.util.AtomicWords; +import org.eclipse.jetty.util.AtomicTriInteger; import org.eclipse.jetty.util.BlockingArrayQueue; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.annotation.ManagedAttribute; @@ -49,7 +49,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP { private static final Logger LOG = Log.getLogger(QueuedThreadPool.class); - private final AtomicWords _counts = new AtomicWords(); + private final AtomicTriInteger _counts = new AtomicTriInteger(); private final AtomicLong _lastShrink = new AtomicLong(); private final Set _threads = ConcurrentHashMap.newKeySet(); private final Object _joinLock = new Object(); @@ -139,7 +139,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP addBean(_tryExecutor); super.doStart(); - _counts.set(0,0,0,0); + _counts.set(0,0,0); ensureThreads(); } @@ -509,7 +509,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @ManagedAttribute("number of idle threads in the pool") public int getIdleThreads() { - return _counts.getWord3(); + return _counts.getWord2(); } /** @@ -544,16 +544,16 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP while (isRunning()) { long count = _counts.get(); - int threads = AtomicWords.getWord0(count); - int starting = AtomicWords.getWord1(count); - int idle = AtomicWords.getWord3(count); + int threads = AtomicTriInteger.getWord0(count); + int starting = AtomicTriInteger.getWord1(count); + int idle = AtomicTriInteger.getWord2(count); int queue = getQueueSize(); if (threads >= _maxThreads) break; if (threads >= _minThreads && (starting + idle) >= queue) break; - if (!_counts.compareAndSet(count, threads + 1, starting + 1, 0, idle)) + if (!_counts.compareAndSet(count, threads + 1, starting + 1, idle)) continue; boolean started = false; @@ -576,7 +576,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP finally { if (!started) - _counts.add(-1,-1,0,0); + _counts.add(-1,-1,0); } } } @@ -669,9 +669,9 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP public String toString() { long count = _counts.get(); - int threads = AtomicWords.getWord0(count); - int starting = AtomicWords.getWord1(count); - int idle = AtomicWords.getWord3(count); + int threads = AtomicTriInteger.getWord0(count); + int starting = AtomicTriInteger.getWord1(count); + int idle = AtomicTriInteger.getWord2(count); int queue = getQueueSize(); return String.format("%s[%s]@%x{%s,%d<=%d<=%d,s=%d,i=%d,r=%d,q=%d}[%s]", @@ -773,7 +773,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP { job = _jobs.poll(); idle = job==null; - _counts.add(0,-1,0,idle?1:0); + _counts.add(0,-1,idle?1:0); if (LOG.isDebugEnabled()) LOG.debug("Runner started with {} for {}", job, QueuedThreadPool.this); @@ -785,7 +785,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP if (!idle) { idle = true; - _counts.add(0,0,0,1); + _counts.add(0,0,1); } job = idleJobPoll(); @@ -813,7 +813,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP if (idle) { idle = false; - _counts.add(0,0,0,-1); + _counts.add(0,0,-1); } if (LOG.isDebugEnabled()) @@ -844,7 +844,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP } finally { - _counts.add(-1,0,0,idle?-1:0); + _counts.add(-1,0,idle?-1:0); removeThread(Thread.currentThread()); ensureThreads(); diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/AtomicTriIntegerTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/AtomicTriIntegerTest.java new file mode 100644 index 00000000000..cd1bd5ade8c --- /dev/null +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/AtomicTriIntegerTest.java @@ -0,0 +1,103 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.util; + +import org.junit.jupiter.api.Test; + +import static org.eclipse.jetty.util.AtomicTriInteger.MAX_VALUE; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class AtomicTriIntegerTest +{ + + @Test + public void testBitOperations() + { + long encoded; + + encoded = AtomicTriInteger.encode(0,0,0); + assertThat(AtomicTriInteger.getWord0(encoded),is(0)); + assertThat(AtomicTriInteger.getWord1(encoded),is(0)); + assertThat(AtomicTriInteger.getWord2(encoded),is(0)); + + encoded = AtomicTriInteger.encode(1,2,3); + assertThat(AtomicTriInteger.getWord0(encoded),is(1)); + assertThat(AtomicTriInteger.getWord1(encoded),is(2)); + assertThat(AtomicTriInteger.getWord2(encoded),is(3)); + + encoded = AtomicTriInteger.encode(MAX_VALUE, MAX_VALUE, MAX_VALUE); + assertThat(AtomicTriInteger.getWord0(encoded),is(MAX_VALUE)); + assertThat(AtomicTriInteger.getWord1(encoded),is(MAX_VALUE)); + assertThat(AtomicTriInteger.getWord2(encoded),is(MAX_VALUE)); + + assertThrows(IllegalArgumentException.class,()-> AtomicTriInteger.encode(-1, MAX_VALUE, MAX_VALUE)); + assertThrows(IllegalArgumentException.class,()-> AtomicTriInteger.encode(MAX_VALUE, -1, MAX_VALUE)); + assertThrows(IllegalArgumentException.class,()-> AtomicTriInteger.encode(MAX_VALUE, MAX_VALUE, -1)); + assertThrows(IllegalArgumentException.class,()-> AtomicTriInteger.encode(MAX_VALUE+1, MAX_VALUE, MAX_VALUE)); + assertThrows(IllegalArgumentException.class,()-> AtomicTriInteger.encode(MAX_VALUE, MAX_VALUE+1, MAX_VALUE)); + assertThrows(IllegalArgumentException.class,()-> AtomicTriInteger.encode(MAX_VALUE, MAX_VALUE, MAX_VALUE+1)); + } + + @Test + public void testSetGet() + { + AtomicTriInteger ati = new AtomicTriInteger(); + ati.set(1,2,3); + assertThat(ati.getWord0(),is(1)); + assertThat(ati.getWord1(),is(2)); + assertThat(ati.getWord2(),is(3)); + } + + @Test + public void testCompareAndSet() + { + AtomicTriInteger ati = new AtomicTriInteger(); + ati.set(1,2,3); + long value = ati.get(); + + ati.set(2,3,4); + assertFalse(ati.compareAndSet(value,5,6,7)); + assertThat(ati.getWord0(),is(2)); + assertThat(ati.getWord1(),is(3)); + assertThat(ati.getWord2(),is(4)); + + value = ati.get(); + assertTrue(ati.compareAndSet(value,6,7,8)); + assertThat(ati.getWord0(),is(6)); + assertThat(ati.getWord1(),is(7)); + assertThat(ati.getWord2(),is(8)); + } + + + @Test + public void testAdd() + { + AtomicTriInteger ati = new AtomicTriInteger(); + ati.set(1,2,3); + ati.add(-1,-2,4); + assertThat(ati.getWord0(),is(0)); + assertThat(ati.getWord1(),is(0)); + assertThat(ati.getWord2(),is(7)); + } + +} From 3c51304b0862c25b2232e29f6e480677e87bde26 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 29 Apr 2019 07:41:59 +0200 Subject: [PATCH 07/17] updates from review Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/util/thread/QueuedThreadPool.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 7289a88930b..898fdb0edc5 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -49,6 +49,13 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP { private static final Logger LOG = Log.getLogger(QueuedThreadPool.class); + /** + * Encodes thread counts:
+ *
Word0
Total thread count (including starting and idle)
+ *
Word1
Starting threads
+ *
Word2
Idle threads
+ *
+ */ private final AtomicTriInteger _counts = new AtomicTriInteger(); private final AtomicLong _lastShrink = new AtomicLong(); private final Set _threads = ConcurrentHashMap.newKeySet(); @@ -280,7 +287,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override public void setMaxThreads(int maxThreads) { - if (maxThreads<0 || maxThreads>0x7FFF) + if (maxThreadsAtomicTriInteger.MAX_VALUE) throw new IllegalArgumentException("maxThreads="+maxThreads); if (_budget!=null) _budget.check(maxThreads); From 5496f72a592e8c92a4ca4df5c42cc11b1175f2b5 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 29 Apr 2019 16:33:08 +0200 Subject: [PATCH 08/17] set reserved thread idle timeout Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/util/thread/QueuedThreadPool.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 898fdb0edc5..1341513f9d2 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -142,7 +142,14 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override protected void doStart() throws Exception { - _tryExecutor = _reservedThreads==0 ? NO_TRY : new ReservedThreadExecutor(this,_reservedThreads); + if (_reservedThreads<=0) + _tryExecutor = NO_TRY; + else + { + ReservedThreadExecutor reserved = new ReservedThreadExecutor(this,_reservedThreads); + reserved.setIdleTimeout(_idleTimeout,TimeUnit.MILLISECONDS); + _tryExecutor = reserved; + } addBean(_tryExecutor); super.doStart(); From 4dd76fc092c09cfc526abffb542cc750e918c158 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 8 May 2019 14:28:20 +0200 Subject: [PATCH 09/17] Issue #3550 Cleanup QTP after review Signed-off-by: Greg Wilkins --- .../jetty/util/thread/QueuedThreadPool.java | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index abe1369245d..914876d24f7 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -153,7 +153,8 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP addBean(_tryExecutor); super.doStart(); - _counts.set(0,0,0); + + _counts.set(0,0,0); // threads, starting, idle ensureThreads(); } @@ -177,7 +178,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP // Fill job Q with noop jobs to wakeup idle Runnable noop = () -> {}; - for (int i = _counts.getWord0(); i-- > 0; ) + for (int i = getThreads(); i-- > 0; ) jobs.offer(noop); // try to let jobs complete naturally for half our stop time @@ -561,17 +562,17 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP { while (isRunning()) { - long count = _counts.get(); - int threads = AtomicTriInteger.getWord0(count); - int starting = AtomicTriInteger.getWord1(count); - int idle = AtomicTriInteger.getWord2(count); + long counts = _counts.get(); + int threads = AtomicTriInteger.getWord0(counts); + int starting = AtomicTriInteger.getWord1(counts); + int idle = AtomicTriInteger.getWord2(counts); int queue = getQueueSize(); if (threads >= _maxThreads) break; if (threads >= _minThreads && (starting + idle) >= queue) break; - if (!_counts.compareAndSet(count, threads + 1, starting + 1, idle)) + if (!_counts.compareAndSet(counts, threads + 1, starting + 1, idle)) continue; boolean started = false; @@ -594,7 +595,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP finally { if (!started) - _counts.add(-1,-1,0); + _counts.add(-1,-1,0); // threads, starting, idle } } } @@ -791,7 +792,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP { job = _jobs.poll(); idle = job==null; - _counts.add(0,-1,idle?1:0); + _counts.add(0,-1,idle?1:0); // threads, starting, idle if (LOG.isDebugEnabled()) LOG.debug("Runner started with {} for {}", job, QueuedThreadPool.this); @@ -803,7 +804,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP if (!idle) { idle = true; - _counts.add(0,0,1); + _counts.add(0,0,1); // threads, starting, idle } job = idleJobPoll(); @@ -831,7 +832,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP if (idle) { idle = false; - _counts.add(0,0,-1); + _counts.add(0,0,-1); // threads, starting, idle } if (LOG.isDebugEnabled()) @@ -862,7 +863,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP } finally { - _counts.add(-1,0,idle?-1:0); + _counts.add(-1,0,idle?-1:0); // threads, starting, idle removeThread(Thread.currentThread()); ensureThreads(); From 715939217f72aaf2be188091cb587f6c8942b13f Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 9 May 2019 16:27:00 +0200 Subject: [PATCH 10/17] Fixed reserved thread default Signed-off-by: Greg Wilkins --- .../java/org/eclipse/jetty/util/thread/QueuedThreadPool.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 914876d24f7..ab552fd0d77 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -142,7 +142,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override protected void doStart() throws Exception { - if (_reservedThreads<=0) + if (_reservedThreads==0) _tryExecutor = NO_TRY; else { From 2b227f121ea6d416d55ab4a5f6e86c27981d5cf7 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 17 May 2019 15:35:18 +0200 Subject: [PATCH 11/17] Issue #3550 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I have modified the JMH benchmark to look at latency (by measuring throughput) for executing a job with low, medium and high concurrency. It looks like the JREs threadpool is not so bad in some load ranges, but once things get busy we are still a bit better. No significant difference is seen between previous QTP impl and the one in this PR. ``` Benchmark (size) (type) Mode Cnt Score Error Units ThreadPoolBenchmark.testFew 200 ETP thrpt 3 129113.271 ± 10821.235 ops/s ThreadPoolBenchmark.testFew 200 QTP thrpt 3 122970.794 ± 8702.327 ops/s ThreadPoolBenchmark.testFew 200 QTP+ thrpt 3 121408.662 ± 12420.318 ops/s ThreadPoolBenchmark.testSome 200 ETP thrpt 3 277400.574 ± 34433.710 ops/s ThreadPoolBenchmark.testSome 200 QTP thrpt 3 244056.673 ± 60118.319 ops/s ThreadPoolBenchmark.testSome 200 QTP+ thrpt 3 240686.913 ± 43104.941 ops/s ThreadPoolBenchmark.testMany 200 ETP thrpt 3 679336.308 ± 157389.044 ops/s ThreadPoolBenchmark.testMany 200 QTP thrpt 3 704502.083 ± 15624.325 ops/s ThreadPoolBenchmark.testMany 200 QTP+ thrpt 3 708220.737 ± 3254.264 ops/s ``` Signed-off-by: Greg Wilkins --- .../util/thread/jmh/ThreadPoolBenchmark.java | 65 +++++++++---------- 1 file changed, 31 insertions(+), 34 deletions(-) diff --git a/jetty-jmh/src/main/java/org/eclipse/jetty/util/thread/jmh/ThreadPoolBenchmark.java b/jetty-jmh/src/main/java/org/eclipse/jetty/util/thread/jmh/ThreadPoolBenchmark.java index f986a6217a9..0796286afbc 100644 --- a/jetty-jmh/src/main/java/org/eclipse/jetty/util/thread/jmh/ThreadPoolBenchmark.java +++ b/jetty-jmh/src/main/java/org/eclipse/jetty/util/thread/jmh/ThreadPoolBenchmark.java @@ -18,9 +18,7 @@ package org.eclipse.jetty.util.thread.jmh; -import java.util.ArrayList; -import java.util.List; -import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import org.eclipse.jetty.util.component.LifeCycle; @@ -38,17 +36,14 @@ import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.TearDown; import org.openjdk.jmh.annotations.Threads; import org.openjdk.jmh.annotations.Warmup; -import org.openjdk.jmh.infra.Blackhole; import org.openjdk.jmh.runner.Runner; import org.openjdk.jmh.runner.RunnerException; import org.openjdk.jmh.runner.options.Options; import org.openjdk.jmh.runner.options.OptionsBuilder; -import org.openjdk.jmh.runner.options.TimeValue; @State(Scope.Benchmark) -@Threads(4) -@Warmup(iterations = 7, time = 500, timeUnit = TimeUnit.MILLISECONDS) -@Measurement(iterations = 7, time = 500, timeUnit = TimeUnit.MILLISECONDS) +@Warmup(iterations = 5, time = 10000, timeUnit = TimeUnit.MILLISECONDS) +@Measurement(iterations = 3, time = 10000, timeUnit = TimeUnit.MILLISECONDS) public class ThreadPoolBenchmark { public enum Type @@ -59,10 +54,7 @@ public class ThreadPoolBenchmark @Param({ "QTP", "ETP"}) Type type; - @Param({ "50"}) - int tasks; - - @Param({ "200", "2000"}) + @Param({ "200" }) int size; ThreadPool pool; @@ -73,11 +65,11 @@ public class ThreadPoolBenchmark switch(type) { case QTP: - pool = new QueuedThreadPool(size); + pool = new QueuedThreadPool(size,size); break; case ETP: - pool = new ExecutorThreadPool(size); + pool = new ExecutorThreadPool(size,size); break; } LifeCycle.start(pool); @@ -85,9 +77,27 @@ public class ThreadPoolBenchmark @Benchmark @BenchmarkMode(Mode.Throughput) - public void testPool() + @Threads(1) + public void testFew() throws Exception { - doWork().join(); + doJob(); + } + + @Benchmark + @BenchmarkMode(Mode.Throughput) + @Threads(4) + public void testSome() throws Exception + { + doJob(); + } + + + @Benchmark + @BenchmarkMode(Mode.Throughput) + @Threads(200) + public void testMany() throws Exception + { + doJob(); } @TearDown // (Level.Iteration) @@ -97,33 +107,20 @@ public class ThreadPoolBenchmark pool = null; } - CompletableFuture doWork() + void doJob() throws Exception { - List> futures = new ArrayList<>(); - for (int i = 0; i < tasks; i++) - { - final CompletableFuture f = new CompletableFuture(); - futures.add(f); - pool.execute(() -> { - Blackhole.consumeCPU(64); - f.complete(null); - }); - } - - return CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])); + CountDownLatch latch = new CountDownLatch(1); + pool.execute(latch::countDown); + latch.await(); } public static void main(String[] args) throws RunnerException { Options opt = new OptionsBuilder() .include(ThreadPoolBenchmark.class.getSimpleName()) - .warmupIterations(2) - .measurementIterations(3) .forks(1) - .threads(400) + // .threads(400) // .syncIterations(true) // Don't start all threads at same time - .warmupTime(new TimeValue(10000,TimeUnit.MILLISECONDS)) - .measurementTime(new TimeValue(10000,TimeUnit.MILLISECONDS)) // .addProfiler(CompilerProfiler.class) // .addProfiler(LinuxPerfProfiler.class) // .addProfiler(LinuxPerfNormProfiler.class) From b70d22fee88605d23463890b0602484d918d2846 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 22 May 2019 18:17:44 +0200 Subject: [PATCH 12/17] Issue #3550 - QueuedThreadPool cleanup. Improved code formatting. Removed unnecessary code in doStop(). Now explicitly checking whether idleTimeout > 0 before shrinking. Signed-off-by: Simone Bordet --- .../eclipse/jetty/util/AtomicTriInteger.java | 52 +++++++++---------- .../jetty/util/thread/QueuedThreadPool.java | 44 +++++++--------- 2 files changed, 46 insertions(+), 50 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicTriInteger.java b/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicTriInteger.java index e01c96f3cc8..6ecec73d382 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicTriInteger.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicTriInteger.java @@ -27,6 +27,7 @@ public class AtomicTriInteger extends AtomicLong { public static int MAX_VALUE = 0x1FFFFF; public static int MIN_VALUE = 0; + /** * Sets the hi and lo values. * @@ -51,7 +52,7 @@ public class AtomicTriInteger extends AtomicLong */ public boolean compareAndSet(long expectEncoded, int w0, int w1, int w2) { - return compareAndSet(expectEncoded,encode(w0, w1, w2)); + return compareAndSet(expectEncoded, encode(w0, w1, w2)); } /** @@ -63,19 +64,18 @@ public class AtomicTriInteger extends AtomicLong */ public void add(int delta0, int delta1, int delta2) { - while(true) + while (true) { long encoded = get(); long update = encode( - getWord0(encoded)+delta0, - getWord1(encoded)+delta1, - getWord2(encoded)+delta2); - if (compareAndSet(encoded,update)) + getWord0(encoded) + delta0, + getWord1(encoded) + delta1, + getWord2(encoded) + delta2); + if (compareAndSet(encoded, update)) return; } } - /** * Gets word 0 value * @@ -115,7 +115,7 @@ public class AtomicTriInteger extends AtomicLong */ public static int getWord0(long encoded) { - return (int) ((encoded>>42)&0x1FFFFFL); + return (int)((encoded >> 42) & MAX_VALUE); } /** @@ -126,7 +126,7 @@ public class AtomicTriInteger extends AtomicLong */ public static int getWord1(long encoded) { - return (int) ((encoded>>21)&0x1FFFFFL); + return (int)((encoded >> 21) & MAX_VALUE); } /** @@ -137,7 +137,7 @@ public class AtomicTriInteger extends AtomicLong */ public static int getWord2(long encoded) { - return (int) (encoded&0x1FFFFFL); + return (int)(encoded & MAX_VALUE); } /** @@ -150,26 +150,26 @@ public class AtomicTriInteger extends AtomicLong */ public static long encode(int w0, int w1, int w2) { - if (w0MAX_VALUE - || w1MAX_VALUE - || w2MAX_VALUE) - throw new IllegalArgumentException(String.format("Words must be 0<= word <= 0x1FFFFF: %d, %d, %d", w0, w1, w2)); - long wl0 = ((long)w0)&0x1FFFFFL; - long wl1 = ((long)w1)&0x1FFFFFL; - long wl2 = ((long)w2)&0x1FFFFFL; - return (wl0<<42)+(wl1<<21)+(wl2); + if (w0 < MIN_VALUE + || w0 > MAX_VALUE + || w1 < MIN_VALUE + || w1 > MAX_VALUE + || w2 < MIN_VALUE + || w2 > MAX_VALUE) + throw new IllegalArgumentException(String.format("Words must be %d <= word <= %d: %d, %d, %d", MIN_VALUE, MAX_VALUE, w0, w1, w2)); + long wl0 = ((long)w0) & MAX_VALUE; + long wl1 = ((long)w1) & MAX_VALUE; + long wl2 = ((long)w2) & MAX_VALUE; + return (wl0 << 42) + (wl1 << 21) + (wl2); } @Override public String toString() { - long value = get(); - int w0 = getWord0(value); - int w1 = getWord1(value); - int w2 = getWord2(value); - return String.format("{%d,%d,%d}",w0,w1,w2); + long encoded = get(); + int w0 = getWord0(encoded); + int w1 = getWord1(encoded); + int w2 = getWord2(encoded); + return String.format("{%d,%d,%d}", w0, w1, w2); } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index ab552fd0d77..7d11e8e19e5 100755 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -143,7 +143,9 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP protected void doStart() throws Exception { if (_reservedThreads==0) + { _tryExecutor = NO_TRY; + } else { ReservedThreadExecutor reserved = new ReservedThreadExecutor(this,_reservedThreads); @@ -198,28 +200,22 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP joinThreads(System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeout) / 2); Thread.yield(); - int size = _threads.size(); - if (size > 0) + if (LOG.isDebugEnabled()) { - Thread.yield(); - - if (LOG.isDebugEnabled()) + for (Thread unstopped : _threads) { - for (Thread unstopped : _threads) + StringBuilder dmp = new StringBuilder(); + for (StackTraceElement element : unstopped.getStackTrace()) { - StringBuilder dmp = new StringBuilder(); - for (StackTraceElement element : unstopped.getStackTrace()) - { - dmp.append(System.lineSeparator()).append("\tat ").append(element); - } - LOG.warn("Couldn't stop {}{}", unstopped, dmp.toString()); + dmp.append(System.lineSeparator()).append("\tat ").append(element); } + LOG.warn("Couldn't stop {}{}", unstopped, dmp.toString()); } - else - { - for (Thread unstopped : _threads) - LOG.warn("{} Couldn't stop {}",this,unstopped); - } + } + else + { + for (Thread unstopped : _threads) + LOG.warn("{} Couldn't stop {}",this,unstopped); } // Close any un-executed jobs @@ -784,7 +780,6 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override public void run() { - boolean idle = false; Runnable job = null; @@ -807,14 +802,15 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP _counts.add(0,0,1); // threads, starting, idle } - job = idleJobPoll(); + long idleTimeout = getIdleTimeout(); + job = idleJobPoll(idleTimeout); // maybe we should shrink? - if (job == null && getThreads() > _minThreads) + if (job == null && getThreads() > _minThreads && idleTimeout > 0) { long last = _lastShrink.get(); long now = System.nanoTime(); - if (last == 0 || (now - last) > TimeUnit.MILLISECONDS.toNanos(_idleTimeout)) + if (last == 0 || (now - last) > TimeUnit.MILLISECONDS.toNanos(idleTimeout)) { if (_lastShrink.compareAndSet(last, now)) { @@ -872,11 +868,11 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP } } - private Runnable idleJobPoll() throws InterruptedException + private Runnable idleJobPoll(long idleTimeout) throws InterruptedException { - if (_idleTimeout <= 0) + if (idleTimeout <= 0) return _jobs.take(); - return _jobs.poll(_idleTimeout, TimeUnit.MILLISECONDS); + return _jobs.poll(idleTimeout, TimeUnit.MILLISECONDS); } } } From 5d267963a3016d4810d10aa272f512247c0f3b60 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 15 May 2019 13:55:20 -0500 Subject: [PATCH 13/17] Issue #3655 - Cookie generation now complies with RFC6265 spaces Signed-off-by: Joakim Erdfelt --- .../jetty/security/ConstraintTest.java | 62 +++++++++---------- .../eclipse/jetty/server/CookieCutter.java | 24 ++++--- .../org/eclipse/jetty/server/Response.java | 43 ++++++++----- .../jetty/server/CookieCutterTest.java | 48 +++++++++++--- .../eclipse/jetty/server/ResponseTest.java | 55 ++++++++-------- 5 files changed, 142 insertions(+), 90 deletions(-) diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java index 51573bb354f..481e1ce87ca 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java @@ -833,7 +833,7 @@ public class ConstraintTest assertThat(response, containsString("Expires")); assertThat(response, containsString("URI=/ctx/testLoginPage")); - String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("POST /ctx/j_security_check HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -853,7 +853,7 @@ public class ConstraintTest assertThat(response, containsString("Location")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/auth/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -888,7 +888,7 @@ public class ConstraintTest assertThat(response, containsString(" 302 Found")); assertThat(response, containsString("/ctx/testLoginPage")); assertThat(response, containsString("JSESSIONID=")); - String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/testLoginPage HTTP/1.0\r\n"+ "Cookie: JSESSIONID=" + session + "\r\n" + @@ -917,7 +917,7 @@ public class ConstraintTest assertThat(response, containsString("/ctx/auth/info")); assertThat(response, containsString("JSESSIONID=")); assertThat(response, not(containsString("JSESSIONID=" + session))); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -954,7 +954,7 @@ public class ConstraintTest "test_parameter=test_value\r\n"); assertThat(response, containsString(" 302 Found")); assertThat(response, containsString("/ctx/testLoginPage")); - String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/testLoginPage HTTP/1.0\r\n"+ "Cookie: JSESSIONID=" + session + "\r\n" + @@ -980,7 +980,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/auth/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); // sneak in other request response = _connector.getResponse("GET /ctx/auth/other HTTP/1.0\r\n" + @@ -1043,7 +1043,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/auth/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/auth/info;jsessionid="+session+";other HTTP/1.0\r\n" + "\r\n"); @@ -1079,7 +1079,7 @@ public class ConstraintTest //login response = _connector.getResponse("GET /ctx/prog?action=login HTTP/1.0\r\n\r\n"); assertThat(response, startsWith("HTTP/1.1 200 OK")); - String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/prog?x=y HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + "\r\n"); @@ -1093,7 +1093,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 500 Server Error")); if (response.contains("JSESSIONID")) { - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/prog?x=y HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + "\r\n"); @@ -1110,7 +1110,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 500 Server Error")); response = _connector.getResponse("GET /ctx/prog?action=login HTTP/1.0\r\n\r\n"); assertThat(response, startsWith("HTTP/1.1 200 OK")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/prog?x=y HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + "\r\n"); @@ -1122,7 +1122,7 @@ public class ConstraintTest _server.start(); response = _connector.getResponse("GET /ctx/prog?action=loginlogin HTTP/1.0\r\n\r\n"); assertThat(response, startsWith("HTTP/1.1 500 Server Error")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/prog?x=y HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + "\r\n"); @@ -1134,7 +1134,7 @@ public class ConstraintTest _server.start(); response = _connector.getResponse("GET /ctx/prog?action=loginlogout HTTP/1.0\r\n\r\n"); assertThat(response, startsWith("HTTP/1.1 200 OK")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/prog?x=y HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + "\r\n"); @@ -1146,7 +1146,7 @@ public class ConstraintTest _server.start(); response = _connector.getResponse("GET /ctx/prog?action=loginlogoutlogin HTTP/1.0\r\n\r\n"); assertThat(response, startsWith("HTTP/1.1 200 OK")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/prog?x=y HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + "\r\n"); @@ -1162,7 +1162,7 @@ public class ConstraintTest assertThat(response, containsString(" 302 Found")); assertThat(response, containsString("/ctx/testLoginPage")); assertThat(response, containsString("JSESSIONID=")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/testLoginPage HTTP/1.0\r\n"+ "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1180,7 +1180,7 @@ public class ConstraintTest assertThat(response, containsString("/ctx/auth/info")); assertThat(response, containsString("JSESSIONID=")); assertThat(response, not(containsString("JSESSIONID=" + session))); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/prog?action=constraintlogin HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + "\r\n"); @@ -1193,7 +1193,7 @@ public class ConstraintTest assertThat(response, containsString(" 302 Found")); assertThat(response, containsString("/ctx/testLoginPage")); assertThat(response, containsString("JSESSIONID=")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/testLoginPage HTTP/1.0\r\n"+ "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1211,7 +1211,7 @@ public class ConstraintTest assertThat(response, containsString("/ctx/auth/info")); assertThat(response, containsString("JSESSIONID=")); assertThat(response, not(containsString("JSESSIONID=" + session))); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/prog?action=logout HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + "\r\n"); @@ -1307,7 +1307,7 @@ public class ConstraintTest assertThat(response, containsString("Expires")); assertThat(response, containsString("URI=/ctx/testLoginPage")); - String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("POST /ctx/j_security_check HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1327,7 +1327,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/auth/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1347,7 +1347,7 @@ public class ConstraintTest response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n"); // assertThat(response,startsWith("HTTP/1.1 302 ")); // assertThat(response,containsString("testLoginPage")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("POST /ctx/j_security_check HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1358,7 +1358,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/auth/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1377,7 +1377,7 @@ public class ConstraintTest response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n"); // assertThat(response,startsWith("HTTP/1.1 302 ")); // assertThat(response,containsString("testLoginPage")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("POST /ctx/j_security_check HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1388,7 +1388,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/auth/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1419,7 +1419,7 @@ public class ConstraintTest assertThat(response, containsString(" 302 Found")); assertThat(response, containsString("http://wibble.com:8888/ctx/testLoginPage")); - String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + String session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("POST /ctx/j_security_check HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1438,7 +1438,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/auth/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1458,7 +1458,7 @@ public class ConstraintTest response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n"); assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("testLoginPage")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("POST /ctx/j_security_check HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1469,7 +1469,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/auth/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" + @@ -1489,7 +1489,7 @@ public class ConstraintTest response = _connector.getResponse("GET /ctx/starstar/info HTTP/1.0\r\n\r\n"); assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("testLoginPage")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("POST /ctx/j_security_check HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1500,7 +1500,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/starstar/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/starstar/info HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1512,7 +1512,7 @@ public class ConstraintTest response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n"); // assertThat(response,startsWith("HTTP/1.1 302 ")); // assertThat(response,containsString("testLoginPage")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("POST /ctx/j_security_check HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + @@ -1523,7 +1523,7 @@ public class ConstraintTest assertThat(response, startsWith("HTTP/1.1 302 ")); assertThat(response, containsString("Location")); assertThat(response, containsString("/ctx/auth/info")); - session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx")); + session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx")); response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" + "Cookie: JSESSIONID=" + session + "\r\n" + diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java index 5dce1cf791a..2c5fd6b1c45 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java @@ -17,10 +17,10 @@ // package org.eclipse.jetty.server; + import java.util.ArrayList; import java.util.List; import java.util.Locale; - import javax.servlet.http.Cookie; import org.eclipse.jetty.http.CookieCompliance; @@ -28,14 +28,20 @@ import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; -/* ------------------------------------------------------------ */ -/** Cookie parser - *

Optimized stateful cookie parser. Cookies fields are added with the - * {@link #addCookieField(String)} method and parsed on the next subsequent - * call to {@link #getCookies()}. - * If the added fields are identical to those last added (as strings), then the - * cookies are not re parsed. - * +/** + * Cookie parser + *

+ * Optimized stateful {@code Cookie} header parser. + * Does not support {@code Set-Cookie} header parsing. + *

+ *

> + * Cookies fields are added with the {@link #addCookieField(String)} method and + * parsed on the next subsequent call to {@link #getCookies()}. + *

+ *

+ * If the added fields are identical to those last added (as strings), then the + * cookies are not re parsed. + *

*/ public class CookieCutter { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index e74986a951e..a64973f3f2f 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -24,14 +24,12 @@ import java.nio.channels.IllegalSelectorException; import java.util.Collection; import java.util.Collections; import java.util.EnumSet; -import java.util.Iterator; import java.util.List; import java.util.ListIterator; import java.util.Locale; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; import java.util.stream.Collectors; - import javax.servlet.RequestDispatcher; import javax.servlet.ServletOutputStream; import javax.servlet.http.Cookie; @@ -214,47 +212,58 @@ public class Response implements HttpServletResponse if (!old_set_cookie.startsWith(name) || old_set_cookie.length()<= name.length() || old_set_cookie.charAt(name.length())!='=') continue; + CookieCompliance responseCookieCompliance = getHttpChannel().getHttpConfiguration().getResponseCookieCompliance(); + + String expectedDomainSegment = "; Domain="; // default to RFC6265 + if (responseCookieCompliance == CookieCompliance.RFC2965) + expectedDomainSegment = ";Domain="; + String domain = cookie.getDomain(); + //noinspection Duplicates if (domain!=null) { - if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965) + if (responseCookieCompliance == CookieCompliance.RFC2965) { StringBuilder buf = new StringBuilder(); - buf.append(";Domain="); + buf.append(expectedDomainSegment); quoteOnlyOrAppend(buf,domain,isQuoteNeededForCookie(domain)); domain = buf.toString(); } else { - domain = ";Domain="+domain; + domain = expectedDomainSegment+domain; } if (!old_set_cookie.contains(domain)) continue; } - else if (old_set_cookie.contains(";Domain=")) + else if (old_set_cookie.contains(expectedDomainSegment)) continue; + String expectedPathSegment = "; Path="; // default to RFC6265 + if (responseCookieCompliance == CookieCompliance.RFC2965) + expectedPathSegment = ";Path="; String path = cookie.getPath(); + //noinspection Duplicates if (path!=null) { - if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965) + if (responseCookieCompliance == CookieCompliance.RFC2965) { StringBuilder buf = new StringBuilder(); - buf.append(";Path="); + buf.append(expectedPathSegment); quoteOnlyOrAppend(buf,path,isQuoteNeededForCookie(path)); path = buf.toString(); } else { - path = ";Path="+path; + path = expectedPathSegment+path; } if (!old_set_cookie.contains(path)) continue; } - else if (old_set_cookie.contains(";Path=")) + else if (old_set_cookie.contains(expectedPathSegment)) continue; - if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance() == CookieCompliance.RFC2965) + if (responseCookieCompliance == CookieCompliance.RFC2965) i.set(new HttpField(HttpHeader.CONTENT_ENCODING.SET_COOKIE, newRFC2965SetCookie( cookie.getName(), cookie.getValue(), @@ -377,32 +386,32 @@ public class Response implements HttpServletResponse // Append path if (path!=null && path.length()>0) - buf.append(";Path=").append(path); + buf.append("; Path=").append(path); // Append domain if (domain!=null && domain.length()>0) - buf.append(";Domain=").append(domain); + buf.append("; Domain=").append(domain); // Handle max-age and/or expires if (maxAge >= 0) { // Always use expires // This is required as some browser (M$ this means you!) don't handle max-age even with v1 cookies - buf.append(";Expires="); + buf.append("; Expires="); if (maxAge == 0) buf.append(__01Jan1970_COOKIE); else DateGenerator.formatCookieDate(buf, System.currentTimeMillis() + 1000L * maxAge); - buf.append(";Max-Age="); + buf.append("; Max-Age="); buf.append(maxAge); } // add the other fields if (isSecure) - buf.append(";Secure"); + buf.append("; Secure"); if (isHttpOnly) - buf.append(";HttpOnly"); + buf.append("; HttpOnly"); return buf.toString(); } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java index ec534a1af5c..48780de3c0b 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java @@ -18,15 +18,14 @@ package org.eclipse.jetty.server; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.MatcherAssert.assertThat; - import javax.servlet.http.Cookie; import org.eclipse.jetty.http.CookieCompliance; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + public class CookieCutterTest { private Cookie[] parseCookieHeaders(CookieCompliance compliance,String... headers) @@ -64,7 +63,24 @@ public class CookieCutterTest assertThat("Cookies.length", cookies.length, is(1)); assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 1, "/acme"); } - + + /** + * Example from RFC2109 and RFC2965. + *

+ * Lenient parsing, input has no spaces after ';' token. + *

+ */ + @Test + public void testRFC_Single_Lenient_NoSpaces() + { + String rawCookie = "$Version=\"1\";Customer=\"WILE_E_COYOTE\";$Path=\"/acme\""; + + Cookie cookies[] = parseCookieHeaders(CookieCompliance.RFC2965,rawCookie); + + assertThat("Cookies.length", cookies.length, is(1)); + assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 1, "/acme"); + } + /** * Example from RFC2109 and RFC2965 */ @@ -156,7 +172,7 @@ public class CookieCutterTest assertCookie("Cookies[0]", cookies[0], "session_id", "1234\", $Version=\"1", 0, null); assertCookie("Cookies[1]", cookies[1], "session_id", "1111", 0, null); } - + /** * Example from RFC6265 */ @@ -185,7 +201,25 @@ public class CookieCutterTest assertCookie("Cookies[0]", cookies[0], "SID", "31d4d96e407aad42", 0, null); assertCookie("Cookies[1]", cookies[1], "lang", "en-US", 0, null); } - + + /** + * Example from RFC6265. + *

+ * Lenient parsing, input has no spaces after ';' token. + *

+ */ + @Test + public void testRFC6265_SidLangExample_Lenient() + { + String rawCookie = "SID=31d4d96e407aad42;lang=en-US"; + + Cookie cookies[] = parseCookieHeaders(CookieCompliance.RFC6265,rawCookie); + + assertThat("Cookies.length", cookies.length, is(2)); + assertCookie("Cookies[0]", cookies[0], "SID", "31d4d96e407aad42", 0, null); + assertCookie("Cookies[1]", cookies[1], "lang", "en-US", 0, null); + } + /** * Basic name=value, following RFC6265 rules */ diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java index 53d52225560..929a935cec8 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java @@ -33,6 +33,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Enumeration; import java.util.Iterator; +import java.util.List; import java.util.Locale; import javax.servlet.ServletException; import javax.servlet.ServletOutputStream; @@ -73,9 +74,9 @@ import org.junit.jupiter.api.Test; import static java.nio.charset.StandardCharsets.UTF_8; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; -import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; @@ -977,7 +978,7 @@ public class ResponseTest String set = response.getHttpFields().get("Set-Cookie"); - assertEquals("name=value;Path=/path;Domain=domain;Secure;HttpOnly", set); + assertEquals("name=value; Path=/path; Domain=domain; Secure; HttpOnly", set); } @Test @@ -1015,7 +1016,7 @@ public class ResponseTest String set = response.getHttpFields().get("Set-Cookie"); - assertEquals("foo=bar%3Bbaz;Path=/secure", set); + assertEquals("foo=bar%3Bbaz; Path=/secure", set); } /** @@ -1056,8 +1057,8 @@ public class ResponseTest assertNotNull(set); ArrayList list = Collections.list(set); assertThat(list, containsInAnyOrder( - "name=value;Path=/path;Domain=domain;Secure;HttpOnly", - "name2=value2;Path=/path;Domain=domain" + "name=value; Path=/path; Domain=domain; Secure; HttpOnly", + "name2=value2; Path=/path; Domain=domain" )); //get rid of the cookies @@ -1088,15 +1089,17 @@ public class ResponseTest response.replaceCookie(new HttpCookie("Foo","value", "A", "/path")); response.replaceCookie(new HttpCookie("Foo","value")); - assertThat(Collections.list(response.getHttpFields().getValues("Set-Cookie")), - contains( - "Foo=value", - "Foo=value;Path=/path;Domain=A", - "Foo=value;Path=/path;Domain=B", - "Bar=value", - "Bar=value;Path=/left", - "Bar=value;Path=/right" - )); + String[] expected = new String[]{ + "Foo=value", + "Foo=value; Path=/path; Domain=A", + "Foo=value; Path=/path; Domain=B", + "Bar=value", + "Bar=value; Path=/left", + "Bar=value; Path=/right" + }; + + List actual = Collections.list(response.getHttpFields().getValues("Set-Cookie")); + assertThat("HttpCookie order", actual, hasItems(expected)); } @Test @@ -1252,8 +1255,8 @@ public class ResponseTest response.addSetRFC6265Cookie("everything","value","domain","path",0,true,true); Enumeration e =fields.getValues("Set-Cookie"); assertTrue(e.hasMoreElements()); - assertEquals("everything=something;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); - assertEquals("everything=value;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=something; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); + assertEquals("everything=value; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertFalse(e.hasMoreElements()); assertEquals("Thu, 01 Jan 1970 00:00:00 GMT",fields.get("Expires")); assertFalse(e.hasMoreElements()); @@ -1264,9 +1267,9 @@ public class ResponseTest response.addSetRFC6265Cookie("everything","value","domain2","path",0,true,true); e =fields.getValues("Set-Cookie"); assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Path=path;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=other; Path=path; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertTrue(e.hasMoreElements()); - assertEquals("everything=value;Path=path;Domain=domain2;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=value; Path=path; Domain=domain2; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertFalse(e.hasMoreElements()); //test cookies with same name, same path, one with domain, one without @@ -1275,9 +1278,9 @@ public class ResponseTest response.addSetRFC6265Cookie("everything","value","","path",0,true,true); e =fields.getValues("Set-Cookie"); assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Path=path;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=other; Path=path; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertTrue(e.hasMoreElements()); - assertEquals("everything=value;Path=path;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=value; Path=path; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertFalse(e.hasMoreElements()); @@ -1287,9 +1290,9 @@ public class ResponseTest response.addSetRFC6265Cookie("everything","value","domain1","path2",0,true,true); e =fields.getValues("Set-Cookie"); assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Path=path1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=other; Path=path1; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertTrue(e.hasMoreElements()); - assertEquals("everything=value;Path=path2;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=value; Path=path2; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertFalse(e.hasMoreElements()); //test cookies with same name, same domain, one with path, one without @@ -1298,9 +1301,9 @@ public class ResponseTest response.addSetRFC6265Cookie("everything","value","domain1","",0,true,true); e =fields.getValues("Set-Cookie"); assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Path=path1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=other; Path=path1; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertTrue(e.hasMoreElements()); - assertEquals("everything=value;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=value; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertFalse(e.hasMoreElements()); //test cookies same name only, no path, no domain @@ -1309,8 +1312,8 @@ public class ResponseTest response.addSetRFC6265Cookie("everything","value","","",0,true,true); e =fields.getValues("Set-Cookie"); assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); - assertEquals("everything=value;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly",e.nextElement()); + assertEquals("everything=other; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); + assertEquals("everything=value; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); assertFalse(e.hasMoreElements()); String badNameExamples[] = { From 2de5456790e2717b8117a097b7bb4894837951db Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 15 May 2019 15:23:58 -0500 Subject: [PATCH 14/17] Issue #3655 - Fixing javadoc Signed-off-by: Joakim Erdfelt --- .../src/main/java/org/eclipse/jetty/server/CookieCutter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java index 2c5fd6b1c45..bc51fccdd62 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java @@ -34,7 +34,7 @@ import org.eclipse.jetty.util.log.Logger; * Optimized stateful {@code Cookie} header parser. * Does not support {@code Set-Cookie} header parsing. *

- *

> + *

* Cookies fields are added with the {@link #addCookieField(String)} method and * parsed on the next subsequent call to {@link #getCookies()}. *

From 26169491c9c6bf5a00cca26a357c962691548ff8 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 16 May 2019 13:08:03 +0200 Subject: [PATCH 15/17] Issue #3655 simplifications of Cookie handling Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/http/HttpCookie.java | 227 ++++++++++ .../eclipse/jetty/http/QuotedCSVParser.java | 8 + .../eclipse/jetty/http/HttpCookieTest.java | 187 ++++++++ .../org/eclipse/jetty/server/Response.java | 406 ++---------------- .../eclipse/jetty/server/ResponseTest.java | 332 ++------------ 5 files changed, 495 insertions(+), 665 deletions(-) create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java index 62ab47e9c55..97fe1a447cc 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java @@ -18,10 +18,17 @@ package org.eclipse.jetty.http; +import java.util.List; import java.util.concurrent.TimeUnit; +import org.eclipse.jetty.util.QuotedStringTokenizer; + public class HttpCookie { + // TODO consider replacing this with java.net.HttpCookie + private static final String __COOKIE_DELIM="\",;\\ \t"; + private static final String __01Jan1970_COOKIE = DateGenerator.formatCookieDate(0).trim(); + private final String _name; private final String _value; private final String _comment; @@ -67,6 +74,29 @@ public class HttpCookie _expiration = maxAge < 0 ? -1 : System.nanoTime() + TimeUnit.SECONDS.toNanos(maxAge); } + enum State { START, NAME, VALUE, QUOTED} + + public HttpCookie(String setCookie) + { + List cookies = java.net.HttpCookie.parse(setCookie); + if (cookies.size()!=1) + throw new IllegalStateException(); + + java.net.HttpCookie cookie = cookies.get(0); + + _name = cookie.getName(); + _value = cookie.getValue(); + _domain = cookie.getDomain(); + _path = cookie.getPath(); + _maxAge = cookie.getMaxAge(); + _httpOnly = cookie.isHttpOnly(); + _secure = cookie.getSecure(); + _comment = cookie.getComment(); + _version = cookie.getVersion(); + _expiration = _maxAge < 0 ? -1 : System.nanoTime() + TimeUnit.SECONDS.toNanos(_maxAge); + } + + /** * @return the cookie name */ @@ -161,4 +191,201 @@ public class HttpCookie builder.append(";$Path=").append(getPath()); return builder.toString(); } + + + private static void quoteOnlyOrAppend(StringBuilder buf, String s, boolean quote) + { + if (quote) + QuotedStringTokenizer.quoteOnly(buf,s); + else + buf.append(s); + } + + /* ------------------------------------------------------------ */ + /** Does a cookie value need to be quoted? + * @param s value string + * @return true if quoted; + * @throws IllegalArgumentException If there a control characters in the string + */ + private static boolean isQuoteNeededForCookie(String s) + { + if (s==null || s.length()==0) + return true; + + if (QuotedStringTokenizer.isQuoted(s)) + return false; + + for (int i=0;i=0) + return true; + + if (c<0x20 || c>=0x7f) + throw new IllegalArgumentException("Illegal character in cookie value"); + } + + return false; + } + + public String getSetCookie(CookieCompliance compliance) + { + switch(compliance) + { + case RFC2965: + return getRFC2965SetCookie(); + case RFC6265: + return getRFC6265SetCookie(); + default: + throw new IllegalStateException(); + } + } + + public String getRFC2965SetCookie() + { + // Check arguments + if (_name == null || _name.length() == 0) + throw new IllegalArgumentException("Bad cookie name"); + + // Format value and params + StringBuilder buf = new StringBuilder(); + + // Name is checked for legality by servlet spec, but can also be passed directly so check again for quoting + boolean quote_name=isQuoteNeededForCookie(_name); + quoteOnlyOrAppend(buf,_name,quote_name); + + buf.append('='); + + // Append the value + boolean quote_value=isQuoteNeededForCookie(_value); + quoteOnlyOrAppend(buf,_value,quote_value); + + // Look for domain and path fields and check if they need to be quoted + boolean has_domain = _domain!=null && _domain.length()>0; + boolean quote_domain = has_domain && isQuoteNeededForCookie(_domain); + boolean has_path = _path!=null && _path.length()>0; + boolean quote_path = has_path && isQuoteNeededForCookie(_path); + + // Upgrade the version if we have a comment or we need to quote value/path/domain or if they were already quoted + int version = _version; + if (version==0 && ( _comment!=null || quote_name || quote_value || quote_domain || quote_path || + QuotedStringTokenizer.isQuoted(_name) || QuotedStringTokenizer.isQuoted(_value) || + QuotedStringTokenizer.isQuoted(_path) || QuotedStringTokenizer.isQuoted(_domain))) + version=1; + + // Append version + if (version==1) + buf.append (";Version=1"); + else if (version>1) + buf.append (";Version=").append(version); + + // Append path + if (has_path) + { + buf.append(";Path="); + quoteOnlyOrAppend(buf,_path,quote_path); + } + + // Append domain + if (has_domain) + { + buf.append(";Domain="); + quoteOnlyOrAppend(buf,_domain,quote_domain); + } + + // Handle max-age and/or expires + if (_maxAge >= 0) + { + // Always use expires + // This is required as some browser (M$ this means you!) don't handle max-age even with v1 cookies + buf.append(";Expires="); + if (_maxAge == 0) + buf.append(__01Jan1970_COOKIE); + else + DateGenerator.formatCookieDate(buf, System.currentTimeMillis() + 1000L * _maxAge); + + // for v1 cookies, also send max-age + if (version>=1) + { + buf.append(";Max-Age="); + buf.append(_maxAge); + } + } + + // add the other fields + if (_secure) + buf.append(";Secure"); + if (_httpOnly) + buf.append(";HttpOnly"); + if (_comment != null) + { + buf.append(";Comment="); + quoteOnlyOrAppend(buf,_comment,isQuoteNeededForCookie(_comment)); + } + return buf.toString(); + } + + public String getRFC6265SetCookie() + { + // Check arguments + if (_name == null || _name.length() == 0) + throw new IllegalArgumentException("Bad cookie name"); + + // Name is checked for legality by servlet spec, but can also be passed directly so check again for quoting + // Per RFC6265, Cookie.name follows RFC2616 Section 2.2 token rules + Syntax.requireValidRFC2616Token(_name, "RFC6265 Cookie name"); + // Ensure that Per RFC6265, Cookie.value follows syntax rules + Syntax.requireValidRFC6265CookieValue(_value); + + // Format value and params + StringBuilder buf = new StringBuilder(); + buf.append(_name).append('=').append(_value==null?"":_value); + + // Append path + if (_path!=null && _path.length()>0) + buf.append("; Path=").append(_path); + + // Append domain + if (_domain!=null && _domain.length()>0) + buf.append("; Domain=").append(_domain); + + // Handle max-age and/or expires + if (_maxAge >= 0) + { + // Always use expires + // This is required as some browser (M$ this means you!) don't handle max-age even with v1 cookies + buf.append("; Expires="); + if (_maxAge == 0) + buf.append(__01Jan1970_COOKIE); + else + DateGenerator.formatCookieDate(buf, System.currentTimeMillis() + 1000L * _maxAge); + + buf.append("; Max-Age="); + buf.append(_maxAge); + } + + // add the other fields + if (_secure) + buf.append("; Secure"); + if (_httpOnly) + buf.append("; HttpOnly"); + return buf.toString(); + } + + + public static class SetCookieHttpField extends HttpField + { + final HttpCookie _cookie; + + public SetCookieHttpField(HttpCookie cookie, CookieCompliance compliance) + { + super(HttpHeader.SET_COOKIE, cookie.getSetCookie(compliance)); + this._cookie = cookie; + } + + public HttpCookie getHttpCookie() + { + return _cookie; + } + } } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/QuotedCSVParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/QuotedCSVParser.java index 8f521db7ffb..622c3345e77 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/QuotedCSVParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/QuotedCSVParser.java @@ -18,6 +18,14 @@ package org.eclipse.jetty.http; + +/** + * Implements a quoted comma separated list parser + * in accordance with RFC7230. + * OWS is removed and quoted characters ignored for parsing. + * @see "https://tools.ietf.org/html/rfc7230#section-3.2.6" + * @see "https://tools.ietf.org/html/rfc7230#section-7" + */ public abstract class QuotedCSVParser { private enum State { VALUE, PARAM_NAME, PARAM_VALUE} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java new file mode 100644 index 00000000000..3b55249ba02 --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java @@ -0,0 +1,187 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http; + +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.fail; + +public class HttpCookieTest +{ + + @Test + public void testConstructFromSetCookie() + { + HttpCookie cookie = new HttpCookie("everything=value; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly"); + } + + @Test + public void testSetRFC2965Cookie() throws Exception + { + HttpCookie httpCookie; + + httpCookie = new HttpCookie("null", null, null, null, -1, false, false, null, -1); + assertEquals("null=",httpCookie.getRFC2965SetCookie()); + + + httpCookie = new HttpCookie("minimal", "value", null, null, -1, false, false, null, -1); + assertEquals("minimal=value",httpCookie.getRFC2965SetCookie()); + + httpCookie = new HttpCookie("everything", "something", "domain", "path", 0, true, true, "noncomment", 0); + assertEquals("everything=something;Version=1;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=noncomment",httpCookie.getRFC2965SetCookie()); + + httpCookie = new HttpCookie("everything", "value", "domain", "path", 0, true, true, "comment", 0); + assertEquals("everything=value;Version=1;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",httpCookie.getRFC2965SetCookie()); + + + httpCookie = new HttpCookie("ev erything", "va lue", "do main", "pa th", 1, true, true, "co mment", 1); + String setCookie=httpCookie.getRFC2965SetCookie(); + assertThat(setCookie, Matchers.startsWith("\"ev erything\"=\"va lue\";Version=1;Path=\"pa th\";Domain=\"do main\";Expires=")); + assertThat(setCookie,Matchers.endsWith(" GMT;Max-Age=1;Secure;HttpOnly;Comment=\"co mment\"")); + + httpCookie = new HttpCookie("name", "value", null, null, -1, false, false, null, 0); + setCookie=httpCookie.getRFC2965SetCookie(); + assertEquals(-1,setCookie.indexOf("Version=")); + httpCookie = new HttpCookie("name", "v a l u e", null, null, -1, false, false, null, 0); + setCookie=httpCookie.getRFC2965SetCookie(); + + httpCookie = new HttpCookie("json","{\"services\":[\"cwa\", \"aa\"]}", null, null, -1, false, false, null, -1); + assertEquals("json=\"{\\\"services\\\":[\\\"cwa\\\", \\\"aa\\\"]}\"",httpCookie.getRFC2965SetCookie()); + + httpCookie = new HttpCookie("name", "value%=", null, null, -1, false, false, null, 0); + setCookie=httpCookie.getRFC2965SetCookie(); + assertEquals("name=value%=",setCookie); + } + + @Test + public void testSetRFC6265Cookie() throws Exception + { + // HttpCookie(String name, String value, String domain, String path, long maxAge, boolean httpOnly, boolean secure, String comment, int version) + // httpCookie = new HttpCookie("name", "value", "domain", "path", maxAge, secure, httpOnly); + + HttpCookie httpCookie; + + httpCookie = new HttpCookie("null",null,null,null,-1,false,false, null, -1); + assertEquals("null=",httpCookie.getRFC6265SetCookie()); + + httpCookie = new HttpCookie("minimal","value",null,null,-1,false,false, null, -1); + assertEquals("minimal=value",httpCookie.getRFC6265SetCookie()); + + //test cookies with same name, domain and path + httpCookie = new HttpCookie("everything","something","domain","path",0,true,true, null, -1); + assertEquals("everything=something; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",httpCookie.getRFC6265SetCookie()); + + httpCookie = new HttpCookie("everything","value","domain","path",0,true,true, null, -1); + assertEquals("everything=value; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",httpCookie.getRFC6265SetCookie()); + + String badNameExamples[] = { + "\"name\"", + "name\t", + "na me", + "name\u0082", + "na\tme", + "na;me", + "{name}", + "[name]", + "\"" + }; + + for (String badNameExample : badNameExamples) + { + try + { + httpCookie = new HttpCookie(badNameExample, "value", null, "/", 1, true, true, null, -1); + httpCookie.getRFC6265SetCookie(); + fail(badNameExample); + } + catch (IllegalArgumentException ex) + { + // System.err.printf("%s: %s%n", ex.getClass().getSimpleName(), ex.getMessage()); + assertThat("Testing bad name: [" + badNameExample + "]", ex.getMessage(), + allOf(containsString("RFC6265"), containsString("RFC2616"))); + } + } + + String badValueExamples[] = { + "va\tlue", + "\t", + "value\u0000", + "val\u0082ue", + "va lue", + "va;lue", + "\"value", + "value\"", + "val\\ue", + "val\"ue", + "\"" + }; + + for (String badValueExample : badValueExamples) + { + try + { + httpCookie = new HttpCookie("name", badValueExample, null, "/", 1, true, true, null, -1); + httpCookie.getRFC6265SetCookie(); + fail(); + } + catch (IllegalArgumentException ex) + { + // System.err.printf("%s: %s%n", ex.getClass().getSimpleName(), ex.getMessage()); + assertThat("Testing bad value [" + badValueExample + "]", ex.getMessage(), Matchers.containsString("RFC6265")); + } + } + + String goodNameExamples[] = { + "name", + "n.a.m.e", + "na-me", + "+name", + "na*me", + "na$me", + "#name" + }; + + for (String goodNameExample : goodNameExamples) + { + httpCookie = new HttpCookie(goodNameExample, "value", null, "/", 1, true, true, null, -1); + // should not throw an exception + } + + String goodValueExamples[] = { + "value", + "", + null, + "val=ue", + "val-ue", + "val/ue", + "v.a.l.u.e" + }; + + for (String goodValueExample : goodValueExamples) + { + httpCookie = new HttpCookie("name", goodValueExample, null, "/", 1, true, true, null, -1); + // should not throw an exception + } + } +} diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index a64973f3f2f..86f70732f13 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -40,6 +40,7 @@ import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.DateGenerator; import org.eclipse.jetty.http.HttpContent; import org.eclipse.jetty.http.HttpCookie; +import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpGenerator; @@ -69,12 +70,8 @@ import org.eclipse.jetty.util.log.Logger; public class Response implements HttpServletResponse { private static final Logger LOG = Log.getLogger(Response.class); - private static final String __COOKIE_DELIM="\",;\\ \t"; - private static final String __01Jan1970_COOKIE = DateGenerator.formatCookieDate(0).trim(); private static final int __MIN_BUFFER_SIZE = 1; private static final HttpField __EXPIRES_01JAN1970 = new PreEncodedHttpField(HttpHeader.EXPIRES,DateGenerator.__01Jan1970); - // Cookie building buffer. Reduce garbage for cookie using applications - private static final ThreadLocal __cookieBuilder = ThreadLocal.withInitial(() -> new StringBuilder(128)); public enum OutputType { @@ -168,30 +165,13 @@ public class Response implements HttpServletResponse public void addCookie(HttpCookie cookie) { if (StringUtil.isBlank(cookie.getName())) - { throw new IllegalArgumentException("Cookie.name cannot be blank/null"); - } - - if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965) - addSetRFC2965Cookie( - cookie.getName(), - cookie.getValue(), - cookie.getDomain(), - cookie.getPath(), - cookie.getMaxAge(), - cookie.getComment(), - cookie.isSecure(), - cookie.isHttpOnly(), - cookie.getVersion()); - else - addSetRFC6265Cookie( - cookie.getName(), - cookie.getValue(), - cookie.getDomain(), - cookie.getPath(), - cookie.getMaxAge(), - cookie.isSecure(), - cookie.isHttpOnly()); + + // add the set cookie + _fields.add(new SetCookieHttpField(cookie, getHttpChannel().getHttpConfiguration().getResponseCookieCompliance())); + + // Expire responses with set-cookie headers so they do not get cached. + _fields.put(__EXPIRES_01JAN1970); } /** @@ -207,84 +187,34 @@ public class Response implements HttpServletResponse if (field.getHeader() == HttpHeader.SET_COOKIE) { - String old_set_cookie = field.getValue(); - String name = cookie.getName(); - if (!old_set_cookie.startsWith(name) || old_set_cookie.length()<= name.length() || old_set_cookie.charAt(name.length())!='=') - continue; + CookieCompliance compliance = getHttpChannel().getHttpConfiguration().getResponseCookieCompliance(); - CookieCompliance responseCookieCompliance = getHttpChannel().getHttpConfiguration().getResponseCookieCompliance(); - - String expectedDomainSegment = "; Domain="; // default to RFC6265 - if (responseCookieCompliance == CookieCompliance.RFC2965) - expectedDomainSegment = ";Domain="; - - String domain = cookie.getDomain(); - //noinspection Duplicates - if (domain!=null) - { - if (responseCookieCompliance == CookieCompliance.RFC2965) - { - StringBuilder buf = new StringBuilder(); - buf.append(expectedDomainSegment); - quoteOnlyOrAppend(buf,domain,isQuoteNeededForCookie(domain)); - domain = buf.toString(); - } - else - { - domain = expectedDomainSegment+domain; - } - if (!old_set_cookie.contains(domain)) - continue; - } - else if (old_set_cookie.contains(expectedDomainSegment)) - continue; - - String expectedPathSegment = "; Path="; // default to RFC6265 - if (responseCookieCompliance == CookieCompliance.RFC2965) - expectedPathSegment = ";Path="; - String path = cookie.getPath(); - //noinspection Duplicates - if (path!=null) - { - if (responseCookieCompliance == CookieCompliance.RFC2965) - { - StringBuilder buf = new StringBuilder(); - buf.append(expectedPathSegment); - quoteOnlyOrAppend(buf,path,isQuoteNeededForCookie(path)); - path = buf.toString(); - } - else - { - path = expectedPathSegment+path; - } - if (!old_set_cookie.contains(path)) - continue; - } - else if (old_set_cookie.contains(expectedPathSegment)) - continue; - - if (responseCookieCompliance == CookieCompliance.RFC2965) - i.set(new HttpField(HttpHeader.CONTENT_ENCODING.SET_COOKIE, newRFC2965SetCookie( - cookie.getName(), - cookie.getValue(), - cookie.getDomain(), - cookie.getPath(), - cookie.getMaxAge(), - cookie.getComment(), - cookie.isSecure(), - cookie.isHttpOnly(), - cookie.getVersion()) - )); + HttpCookie oldCookie; + if (field instanceof SetCookieHttpField) + oldCookie = ((SetCookieHttpField)field).getHttpCookie(); else - i.set(new HttpField(HttpHeader.CONTENT_ENCODING.SET_COOKIE, newRFC6265SetCookie( - cookie.getName(), - cookie.getValue(), - cookie.getDomain(), - cookie.getPath(), - cookie.getMaxAge(), - cookie.isSecure(), - cookie.isHttpOnly() - ))); + oldCookie = new HttpCookie(field.getValue()); + + if (!cookie.getName().equals(oldCookie.getName())) + continue; + + if (cookie.getDomain()==null) + { + if (oldCookie.getDomain() != null) + continue; + } + else if (!cookie.getDomain().equalsIgnoreCase(oldCookie.getDomain())) + continue; + + if (cookie.getPath()==null) + { + if (oldCookie.getPath() != null) + continue; + } + else if (!cookie.getPath().equals(oldCookie.getPath())) + continue; + + i.set(new SetCookieHttpField(cookie, compliance)); return; } } @@ -296,8 +226,11 @@ public class Response implements HttpServletResponse @Override public void addCookie(Cookie cookie) { + if (StringUtil.isBlank(cookie.getName())) + throw new IllegalArgumentException("Cookie.name cannot be blank/null"); + String comment = cookie.getComment(); - boolean httpOnly = false; + boolean httpOnly = cookie.isHttpOnly(); if (comment != null) { @@ -310,264 +243,19 @@ public class Response implements HttpServletResponse comment = null; } } - - if (StringUtil.isBlank(cookie.getName())) - { - throw new IllegalArgumentException("Cookie.name cannot be blank/null"); - } - if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965) - addSetRFC2965Cookie(cookie.getName(), - cookie.getValue(), - cookie.getDomain(), - cookie.getPath(), - cookie.getMaxAge(), - comment, - cookie.getSecure(), - httpOnly || cookie.isHttpOnly(), - cookie.getVersion()); - else - addSetRFC6265Cookie(cookie.getName(), - cookie.getValue(), - cookie.getDomain(), - cookie.getPath(), - cookie.getMaxAge(), - cookie.getSecure(), - httpOnly || cookie.isHttpOnly()); + addCookie(new HttpCookie( + cookie.getName(), + cookie.getValue(), + cookie.getDomain(), + cookie.getPath(), + (long) cookie.getMaxAge(), + httpOnly, + cookie.getSecure(), + comment, + cookie.getVersion())); } - - /** - * Format a set cookie value by RFC6265 - * - * @param name the name - * @param value the value - * @param domain the domain - * @param path the path - * @param maxAge the maximum age - * @param isSecure true if secure cookie - * @param isHttpOnly true if for http only - */ - public void addSetRFC6265Cookie( - final String name, - final String value, - final String domain, - final String path, - final long maxAge, - final boolean isSecure, - final boolean isHttpOnly) - { - String set_cookie = newRFC6265SetCookie(name, value, domain, path, maxAge, isSecure, isHttpOnly); - - // add the set cookie - _fields.add(HttpHeader.SET_COOKIE, set_cookie); - - // Expire responses with set-cookie headers so they do not get cached. - _fields.put(__EXPIRES_01JAN1970); - - } - - private String newRFC6265SetCookie(String name, String value, String domain, String path, long maxAge, boolean isSecure, boolean isHttpOnly) - { - // Check arguments - if (name == null || name.length() == 0) - throw new IllegalArgumentException("Bad cookie name"); - - // Name is checked for legality by servlet spec, but can also be passed directly so check again for quoting - // Per RFC6265, Cookie.name follows RFC2616 Section 2.2 token rules - Syntax.requireValidRFC2616Token(name, "RFC6265 Cookie name"); - // Ensure that Per RFC6265, Cookie.value follows syntax rules - Syntax.requireValidRFC6265CookieValue(value); - - // Format value and params - StringBuilder buf = __cookieBuilder.get(); - buf.setLength(0); - buf.append(name).append('=').append(value==null?"":value); - - // Append path - if (path!=null && path.length()>0) - buf.append("; Path=").append(path); - - // Append domain - if (domain!=null && domain.length()>0) - buf.append("; Domain=").append(domain); - - // Handle max-age and/or expires - if (maxAge >= 0) - { - // Always use expires - // This is required as some browser (M$ this means you!) don't handle max-age even with v1 cookies - buf.append("; Expires="); - if (maxAge == 0) - buf.append(__01Jan1970_COOKIE); - else - DateGenerator.formatCookieDate(buf, System.currentTimeMillis() + 1000L * maxAge); - - buf.append("; Max-Age="); - buf.append(maxAge); - } - - // add the other fields - if (isSecure) - buf.append("; Secure"); - if (isHttpOnly) - buf.append("; HttpOnly"); - return buf.toString(); - } - - /** - * Format a set cookie value - * - * @param name the name - * @param value the value - * @param domain the domain - * @param path the path - * @param maxAge the maximum age - * @param comment the comment (only present on versions > 0) - * @param isSecure true if secure cookie - * @param isHttpOnly true if for http only - * @param version version of cookie logic to use (0 == default behavior) - */ - public void addSetRFC2965Cookie( - final String name, - final String value, - final String domain, - final String path, - final long maxAge, - final String comment, - final boolean isSecure, - final boolean isHttpOnly, - int version) - { - String set_cookie = newRFC2965SetCookie(name, value, domain, path, maxAge, comment, isSecure, isHttpOnly, version); - - // add the set cookie - _fields.add(HttpHeader.SET_COOKIE, set_cookie); - - // Expire responses with set-cookie headers so they do not get cached. - _fields.put(__EXPIRES_01JAN1970); - } - - private String newRFC2965SetCookie(String name, String value, String domain, String path, long maxAge, String comment, boolean isSecure, boolean isHttpOnly, int version) - { - // Check arguments - if (name == null || name.length() == 0) - throw new IllegalArgumentException("Bad cookie name"); - - // Format value and params - StringBuilder buf = __cookieBuilder.get(); - buf.setLength(0); - - // Name is checked for legality by servlet spec, but can also be passed directly so check again for quoting - boolean quote_name=isQuoteNeededForCookie(name); - quoteOnlyOrAppend(buf,name,quote_name); - - buf.append('='); - - // Append the value - boolean quote_value=isQuoteNeededForCookie(value); - quoteOnlyOrAppend(buf,value,quote_value); - - // Look for domain and path fields and check if they need to be quoted - boolean has_domain = domain!=null && domain.length()>0; - boolean quote_domain = has_domain && isQuoteNeededForCookie(domain); - boolean has_path = path!=null && path.length()>0; - boolean quote_path = has_path && isQuoteNeededForCookie(path); - - // Upgrade the version if we have a comment or we need to quote value/path/domain or if they were already quoted - if (version==0 && ( comment!=null || quote_name || quote_value || quote_domain || quote_path || - QuotedStringTokenizer.isQuoted(name) || QuotedStringTokenizer.isQuoted(value) || - QuotedStringTokenizer.isQuoted(path) || QuotedStringTokenizer.isQuoted(domain))) - version=1; - - // Append version - if (version==1) - buf.append (";Version=1"); - else if (version>1) - buf.append (";Version=").append(version); - - // Append path - if (has_path) - { - buf.append(";Path="); - quoteOnlyOrAppend(buf,path,quote_path); - } - - // Append domain - if (has_domain) - { - buf.append(";Domain="); - quoteOnlyOrAppend(buf,domain,quote_domain); - } - - // Handle max-age and/or expires - if (maxAge >= 0) - { - // Always use expires - // This is required as some browser (M$ this means you!) don't handle max-age even with v1 cookies - buf.append(";Expires="); - if (maxAge == 0) - buf.append(__01Jan1970_COOKIE); - else - DateGenerator.formatCookieDate(buf, System.currentTimeMillis() + 1000L * maxAge); - - // for v1 cookies, also send max-age - if (version>=1) - { - buf.append(";Max-Age="); - buf.append(maxAge); - } - } - - // add the other fields - if (isSecure) - buf.append(";Secure"); - if (isHttpOnly) - buf.append(";HttpOnly"); - if (comment != null) - { - buf.append(";Comment="); - quoteOnlyOrAppend(buf,comment,isQuoteNeededForCookie(comment)); - } - return buf.toString(); - } - - - /* ------------------------------------------------------------ */ - /** Does a cookie value need to be quoted? - * @param s value string - * @return true if quoted; - * @throws IllegalArgumentException If there a control characters in the string - */ - private static boolean isQuoteNeededForCookie(String s) - { - if (s==null || s.length()==0) - return true; - - if (QuotedStringTokenizer.isQuoted(s)) - return false; - - for (int i=0;i=0) - return true; - - if (c<0x20 || c>=0x7f) - throw new IllegalArgumentException("Illegal character in cookie value"); - } - - return false; - } - - - private static void quoteOnlyOrAppend(StringBuilder buf, String s, boolean quote) - { - if (quote) - QuotedStringTokenizer.quoteOnly(buf,s); - else - buf.append(s); - } @Override public boolean containsHeader(String name) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java index 929a935cec8..2a46b7c7be9 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java @@ -1102,6 +1102,32 @@ public class ResponseTest assertThat("HttpCookie order", actual, hasItems(expected)); } + + @Test + public void testReplaceParsedHttpCookie() + { + Response response = getResponse(); + + response.addHeader(HttpHeader.SET_COOKIE.asString(), "Foo=123456"); + response.replaceCookie(new HttpCookie("Foo","value")); + List actual = Collections.list(response.getHttpFields().getValues("Set-Cookie")); + assertThat(actual, hasItems(new String[] {"Foo=value"})); + + response.setHeader(HttpHeader.SET_COOKIE,"Foo=123456; domain=Bah; Path=/path"); + response.replaceCookie(new HttpCookie("Foo","other")); + actual = Collections.list(response.getHttpFields().getValues("Set-Cookie")); + assertThat(actual, hasItems(new String[] {"Foo=123456; domain=Bah; Path=/path", "Foo=other"})); + + response.replaceCookie(new HttpCookie("Foo","replaced", "Bah", "/path")); + actual = Collections.list(response.getHttpFields().getValues("Set-Cookie")); + assertThat(actual, hasItems(new String[] {"Foo=replaced; Path=/path; Domain=Bah", "Foo=other"})); + + response.setHeader(HttpHeader.SET_COOKIE,"Foo=123456; domain=Bah; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly; Path=/path"); + response.replaceCookie(new HttpCookie("Foo","replaced", "Bah", "/path")); + actual = Collections.list(response.getHttpFields().getValues("Set-Cookie")); + assertThat(actual, hasItems(new String[] {"Foo=replaced; Path=/path; Domain=Bah"})); + } + @Test public void testFlushAfterFullContent() throws Exception { @@ -1114,312 +1140,6 @@ public class ResponseTest // Must not throw output.flush(); } - - @Test - public void testSetRFC2965Cookie() throws Exception - { - Response response = _channel.getResponse(); - HttpFields fields = response.getHttpFields(); - - response.addSetRFC2965Cookie("null",null,null,null,-1,null,false,false,-1); - assertEquals("null=",fields.get("Set-Cookie")); - - fields.clear(); - - response.addSetRFC2965Cookie("minimal","value",null,null,-1,null,false,false,-1); - assertEquals("minimal=value",fields.get("Set-Cookie")); - - fields.clear(); - //test cookies with same name, domain and path - response.addSetRFC2965Cookie("everything","something","domain","path",0,"noncomment",true,true,0); - response.addSetRFC2965Cookie("everything","value","domain","path",0,"comment",true,true,0); - Enumeration e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=something;Version=1;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=noncomment",e.nextElement()); - assertEquals("everything=value;Version=1;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement()); - assertFalse(e.hasMoreElements()); - assertEquals("Thu, 01 Jan 1970 00:00:00 GMT",fields.get("Expires")); - assertFalse(e.hasMoreElements()); - - //test cookies with same name, different domain - fields.clear(); - response.addSetRFC2965Cookie("everything","other","domain1","path",0,"blah",true,true,0); - response.addSetRFC2965Cookie("everything","value","domain2","path",0,"comment",true,true,0); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Version=1;Path=path;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement()); - assertTrue(e.hasMoreElements()); - assertEquals("everything=value;Version=1;Path=path;Domain=domain2;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement()); - assertFalse(e.hasMoreElements()); - - //test cookies with same name, same path, one with domain, one without - fields.clear(); - response.addSetRFC2965Cookie("everything","other","domain1","path",0,"blah",true,true,0); - response.addSetRFC2965Cookie("everything","value","","path",0,"comment",true,true,0); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Version=1;Path=path;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement()); - assertTrue(e.hasMoreElements()); - assertEquals("everything=value;Version=1;Path=path;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement()); - assertFalse(e.hasMoreElements()); - - - //test cookies with same name, different path - fields.clear(); - response.addSetRFC2965Cookie("everything","other","domain1","path1",0,"blah",true,true,0); - response.addSetRFC2965Cookie("everything","value","domain1","path2",0,"comment",true,true,0); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Version=1;Path=path1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement()); - assertTrue(e.hasMoreElements()); - assertEquals("everything=value;Version=1;Path=path2;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement()); - assertFalse(e.hasMoreElements()); - - //test cookies with same name, same domain, one with path, one without - fields.clear(); - response.addSetRFC2965Cookie("everything","other","domain1","path1",0,"blah",true,true,0); - response.addSetRFC2965Cookie("everything","value","domain1","",0,"comment",true,true,0); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Version=1;Path=path1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement()); - assertTrue(e.hasMoreElements()); - assertEquals("everything=value;Version=1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement()); - assertFalse(e.hasMoreElements()); - - //test cookies same name only, no path, no domain - fields.clear(); - response.addSetRFC2965Cookie("everything","other","","",0,"blah",true,true,0); - response.addSetRFC2965Cookie("everything","value","","",0,"comment",true,true,0); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other;Version=1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement()); - assertEquals("everything=value;Version=1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement()); - assertFalse(e.hasMoreElements()); - - fields.clear(); - response.addSetRFC2965Cookie("ev erything","va lue","do main","pa th",1,"co mment",true,true,1); - String setCookie=fields.get("Set-Cookie"); - assertThat(setCookie,Matchers.startsWith("\"ev erything\"=\"va lue\";Version=1;Path=\"pa th\";Domain=\"do main\";Expires=")); - assertThat(setCookie,Matchers.endsWith(" GMT;Max-Age=1;Secure;HttpOnly;Comment=\"co mment\"")); - - fields.clear(); - response.addSetRFC2965Cookie("name","value",null,null,-1,null,false,false,0); - setCookie=fields.get("Set-Cookie"); - assertEquals(-1,setCookie.indexOf("Version=")); - fields.clear(); - response.addSetRFC2965Cookie("name","v a l u e",null,null,-1,null,false,false,0); - setCookie=fields.get("Set-Cookie"); - - fields.clear(); - response.addSetRFC2965Cookie("json","{\"services\":[\"cwa\", \"aa\"]}",null,null,-1,null,false,false,-1); - assertEquals("json=\"{\\\"services\\\":[\\\"cwa\\\", \\\"aa\\\"]}\"",fields.get("Set-Cookie")); - - fields.clear(); - response.addSetRFC2965Cookie("name","value","domain",null,-1,null,false,false,-1); - response.addSetRFC2965Cookie("name","other","domain",null,-1,null,false,false,-1); - response.addSetRFC2965Cookie("name","more","domain",null,-1,null,false,false,-1); - e = fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertThat(e.nextElement(), Matchers.startsWith("name=value")); - assertThat(e.nextElement(), Matchers.startsWith("name=other")); - assertThat(e.nextElement(), Matchers.startsWith("name=more")); - - response.addSetRFC2965Cookie("foo","bar","domain",null,-1,null,false,false,-1); - response.addSetRFC2965Cookie("foo","bob","domain",null,-1,null,false,false,-1); - assertThat(fields.get("Set-Cookie"), Matchers.startsWith("name=value")); - - - fields.clear(); - response.addSetRFC2965Cookie("name","value%=",null,null,-1,null,false,false,0); - setCookie=fields.get("Set-Cookie"); - assertEquals("name=value%=",setCookie); - } - - @Test - public void testSetRFC6265Cookie() throws Exception - { - Response response = _channel.getResponse(); - HttpFields fields = response.getHttpFields(); - - response.addSetRFC6265Cookie("null",null,null,null,-1,false,false); - assertEquals("null=",fields.get("Set-Cookie")); - - fields.clear(); - - response.addSetRFC6265Cookie("minimal","value",null,null,-1,false,false); - assertEquals("minimal=value",fields.get("Set-Cookie")); - - fields.clear(); - //test cookies with same name, domain and path - response.addSetRFC6265Cookie("everything","something","domain","path",0,true,true); - response.addSetRFC6265Cookie("everything","value","domain","path",0,true,true); - Enumeration e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=something; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertEquals("everything=value; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertFalse(e.hasMoreElements()); - assertEquals("Thu, 01 Jan 1970 00:00:00 GMT",fields.get("Expires")); - assertFalse(e.hasMoreElements()); - - //test cookies with same name, different domain - fields.clear(); - response.addSetRFC6265Cookie("everything","other","domain1","path",0,true,true); - response.addSetRFC6265Cookie("everything","value","domain2","path",0,true,true); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other; Path=path; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertTrue(e.hasMoreElements()); - assertEquals("everything=value; Path=path; Domain=domain2; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertFalse(e.hasMoreElements()); - - //test cookies with same name, same path, one with domain, one without - fields.clear(); - response.addSetRFC6265Cookie("everything","other","domain1","path",0,true,true); - response.addSetRFC6265Cookie("everything","value","","path",0,true,true); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other; Path=path; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertTrue(e.hasMoreElements()); - assertEquals("everything=value; Path=path; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertFalse(e.hasMoreElements()); - - - //test cookies with same name, different path - fields.clear(); - response.addSetRFC6265Cookie("everything","other","domain1","path1",0,true,true); - response.addSetRFC6265Cookie("everything","value","domain1","path2",0,true,true); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other; Path=path1; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertTrue(e.hasMoreElements()); - assertEquals("everything=value; Path=path2; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertFalse(e.hasMoreElements()); - - //test cookies with same name, same domain, one with path, one without - fields.clear(); - response.addSetRFC6265Cookie("everything","other","domain1","path1",0,true,true); - response.addSetRFC6265Cookie("everything","value","domain1","",0,true,true); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other; Path=path1; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertTrue(e.hasMoreElements()); - assertEquals("everything=value; Domain=domain1; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertFalse(e.hasMoreElements()); - - //test cookies same name only, no path, no domain - fields.clear(); - response.addSetRFC6265Cookie("everything","other","","",0,true,true); - response.addSetRFC6265Cookie("everything","value","","",0,true,true); - e =fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertEquals("everything=other; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertEquals("everything=value; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly",e.nextElement()); - assertFalse(e.hasMoreElements()); - - String badNameExamples[] = { - "\"name\"", - "name\t", - "na me", - "name\u0082", - "na\tme", - "na;me", - "{name}", - "[name]", - "\"" - }; - - for (String badNameExample : badNameExamples) - { - fields.clear(); - try - { - response.addSetRFC6265Cookie(badNameExample, "value", null, "/", 1, true, true); - } - catch (IllegalArgumentException ex) - { - // System.err.printf("%s: %s%n", ex.getClass().getSimpleName(), ex.getMessage()); - assertThat("Testing bad name: [" + badNameExample + "]", ex.getMessage(), - allOf(containsString("RFC6265"), containsString("RFC2616"))); - } - } - - String badValueExamples[] = { - "va\tlue", - "\t", - "value\u0000", - "val\u0082ue", - "va lue", - "va;lue", - "\"value", - "value\"", - "val\\ue", - "val\"ue", - "\"" - }; - - for (String badValueExample : badValueExamples) - { - fields.clear(); - try - { - response.addSetRFC6265Cookie("name", badValueExample, null, "/", 1, true, true); - } - catch (IllegalArgumentException ex) - { - // System.err.printf("%s: %s%n", ex.getClass().getSimpleName(), ex.getMessage()); - assertThat("Testing bad value [" + badValueExample + "]", ex.getMessage(), Matchers.containsString("RFC6265")); - } - } - - String goodNameExamples[] = { - "name", - "n.a.m.e", - "na-me", - "+name", - "na*me", - "na$me", - "#name" - }; - - for (String goodNameExample : goodNameExamples) - { - fields.clear(); - response.addSetRFC6265Cookie(goodNameExample, "value", null, "/", 1, true, true); - // should not throw an exception - } - - String goodValueExamples[] = { - "value", - "", - null, - "val=ue", - "val-ue", - "val/ue", - "v.a.l.u.e" - }; - - for (String goodValueExample : goodValueExamples) - { - fields.clear(); - response.addSetRFC6265Cookie("name", goodValueExample, null, "/", 1, true, true); - // should not throw an exception - } - - fields.clear(); - - response.addSetRFC6265Cookie("name","value","domain",null,-1,false,false); - response.addSetRFC6265Cookie("name","other","domain",null,-1,false,false); - response.addSetRFC6265Cookie("name","more","domain",null,-1,false,false); - e = fields.getValues("Set-Cookie"); - assertTrue(e.hasMoreElements()); - assertThat(e.nextElement(), Matchers.startsWith("name=value")); - assertThat(e.nextElement(), Matchers.startsWith("name=other")); - assertThat(e.nextElement(), Matchers.startsWith("name=more")); - - response.addSetRFC6265Cookie("foo","bar","domain",null,-1,false,false); - response.addSetRFC6265Cookie("foo","bob","domain",null,-1,false,false); - assertThat(fields.get("Set-Cookie"), Matchers.startsWith("name=value")); - } private Response getResponse() { From 065581edd9f3e7fce8db3ab3af4c2358ee698889 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 17 May 2019 10:33:58 +0200 Subject: [PATCH 16/17] cleanups Signed-off-by: Greg Wilkins --- .../src/main/java/org/eclipse/jetty/http/HttpCookie.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java index 97fe1a447cc..79a7f7daee3 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java @@ -23,9 +23,9 @@ import java.util.concurrent.TimeUnit; import org.eclipse.jetty.util.QuotedStringTokenizer; +// TODO consider replacing this with java.net.HttpCookie public class HttpCookie { - // TODO consider replacing this with java.net.HttpCookie private static final String __COOKIE_DELIM="\",;\\ \t"; private static final String __01Jan1970_COOKIE = DateGenerator.formatCookieDate(0).trim(); @@ -74,8 +74,6 @@ public class HttpCookie _expiration = maxAge < 0 ? -1 : System.nanoTime() + TimeUnit.SECONDS.toNanos(maxAge); } - enum State { START, NAME, VALUE, QUOTED} - public HttpCookie(String setCookie) { List cookies = java.net.HttpCookie.parse(setCookie); @@ -201,7 +199,6 @@ public class HttpCookie buf.append(s); } - /* ------------------------------------------------------------ */ /** Does a cookie value need to be quoted? * @param s value string * @return true if quoted; From 0bf138f034e8cb71b4b4800ffebeb6e5f31bf7d8 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 22 May 2019 15:20:41 +0200 Subject: [PATCH 17/17] removed unused lines Signed-off-by: Greg Wilkins --- .../src/test/java/org/eclipse/jetty/http/HttpCookieTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java index 3b55249ba02..c1ac8685311 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java @@ -77,9 +77,6 @@ public class HttpCookieTest @Test public void testSetRFC6265Cookie() throws Exception { - // HttpCookie(String name, String value, String domain, String path, long maxAge, boolean httpOnly, boolean secure, String comment, int version) - // httpCookie = new HttpCookie("name", "value", "domain", "path", maxAge, secure, httpOnly); - HttpCookie httpCookie; httpCookie = new HttpCookie("null",null,null,null,-1,false,false, null, -1);