diff --git a/.gitignore b/.gitignore index a3cbb20fda1..847f4383974 100644 --- a/.gitignore +++ b/.gitignore @@ -19,6 +19,7 @@ bin/ *.backup *.debug *.dump +.attach_pid* # vim .*.sw[a-p] 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..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 @@ -18,10 +18,17 @@ package org.eclipse.jetty.http; +import java.util.List; import java.util.concurrent.TimeUnit; +import org.eclipse.jetty.util.QuotedStringTokenizer; + +// TODO consider replacing this with java.net.HttpCookie public class 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,27 @@ public class HttpCookie _expiration = maxAge < 0 ? -1 : System.nanoTime() + TimeUnit.SECONDS.toNanos(maxAge); } + 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 +189,200 @@ 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..c1ac8685311 --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java @@ -0,0 +1,184 @@ +// +// ======================================================================== +// 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 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-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) 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..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 @@ -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 1f73a14940c..f5733470fb2 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 @@ -39,6 +39,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; @@ -68,12 +69,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 { @@ -167,30 +164,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); } /** @@ -206,73 +186,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(); - String domain = cookie.getDomain(); - if (domain!=null) - { - if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965) - { - StringBuilder buf = new StringBuilder(); - buf.append(";Domain="); - quoteOnlyOrAppend(buf,domain,isQuoteNeededForCookie(domain)); - domain = buf.toString(); - } - else - { - domain = ";Domain="+domain; - } - if (!old_set_cookie.contains(domain)) - continue; - } - else if (old_set_cookie.contains(";Domain=")) - continue; - - String path = cookie.getPath(); - if (path!=null) - { - if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965) - { - StringBuilder buf = new StringBuilder(); - buf.append(";Path="); - quoteOnlyOrAppend(buf,path,isQuoteNeededForCookie(path)); - path = buf.toString(); - } - else - { - path = ";Path="+path; - } - if (!old_set_cookie.contains(path)) - continue; - } - else if (old_set_cookie.contains(";Path=")) - continue; - - if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance() == 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; } } @@ -284,8 +225,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) { @@ -298,264 +242,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/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..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 @@ -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,43 @@ 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 + 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 @@ -1111,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() { 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 new file mode 100644 index 00000000000..6ecec73d382 --- /dev/null +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/AtomicTriInteger.java @@ -0,0 +1,175 @@ +// +// ======================================================================== +// 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 three 21 bit unsigned words. + */ +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 + */ + public void set(int w0, int w1, int w2) + { + set(encode(w0, w1, w2)); + } + + /** + * 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 + * @return {@code true} if successful. + */ + public boolean compareAndSet(long expectEncoded, int w0, int w1, int w2) + { + return compareAndSet(expectEncoded, encode(w0, w1, w2)); + } + + /** + * 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 + */ + public void add(int delta0, int delta1, int delta2) + { + while (true) + { + long encoded = get(); + long update = encode( + getWord0(encoded) + delta0, + getWord1(encoded) + delta1, + getWord2(encoded) + delta2); + 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 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 >> 42) & MAX_VALUE); + } + + /** + * 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 >> 21) & MAX_VALUE); + } + + /** + * 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 & MAX_VALUE); + } + + /** + * Encodes 4 16 bit words values into a long. + * + * @param w0 the 0th word + * @param w1 the 1st word + * @param w2 the 2nd word + * @return the encoded value + */ + public static long encode(int w0, int w1, int w2) + { + 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 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 796ae1b646b..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 @@ -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.AtomicTriInteger; import org.eclipse.jetty.util.BlockingArrayQueue; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.annotation.ManagedAttribute; @@ -49,8 +49,14 @@ 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(); + /** + * 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(); private final Object _joinLock = new Object(); @@ -136,13 +142,22 @@ 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(); - _threadsStarted.set(0); - startThreads(_minThreads); + _counts.set(0,0,0); // threads, starting, idle + ensureThreads(); } @Override @@ -165,65 +180,42 @@ 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 = getThreads(); 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) + 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 @@ -254,6 +246,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. * @@ -287,6 +291,8 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override public void setMaxThreads(int maxThreads) { + if (maxThreadsAtomicTriInteger.MAX_VALUE) + throw new IllegalArgumentException("maxThreads="+maxThreads); if (_budget!=null) _budget.check(maxThreads); _maxThreads = maxThreads; @@ -308,9 +314,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(); } /** @@ -468,19 +473,15 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @Override public void execute(Runnable job) { - if (LOG.isDebugEnabled()) - LOG.debug("queue {}",job); if (!isRunning() || !_jobs.offer(job)) { 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); - } + if (LOG.isDebugEnabled()) + LOG.debug("queue {}",job); + // Make sure there is at least one thread executing the job. + ensureThreads(); } @Override @@ -513,7 +514,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @ManagedAttribute("number of threads in the pool") public int getThreads() { - return _threadsStarted.get(); + return _counts.getWord0(); } /** @@ -523,7 +524,7 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP @ManagedAttribute("number of idle threads in the pool") public int getIdleThreads() { - return _threadsIdle.get(); + return _counts.getWord2(); } /** @@ -553,20 +554,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 counts = _counts.get(); + int threads = AtomicTriInteger.getWord0(counts); + int starting = AtomicTriInteger.getWord1(counts); + int idle = AtomicTriInteger.getWord2(counts); + int queue = getQueueSize(); - if (!_threadsStarted.compareAndSet(threads, threads + 1)) + if (threads >= _maxThreads) + break; + if (threads >= _minThreads && (starting + idle) >= queue) + break; + if (!_counts.compareAndSet(counts, threads + 1, starting + 1, idle)) continue; boolean started = false; try { + if (LOG.isDebugEnabled()) + LOG.debug("Starting thread {}",this); + Thread thread = newThread(_runnable); thread.setDaemon(isDaemon()); thread.setPriority(getThreadsPriority()); @@ -577,15 +587,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); // threads, starting, idle } } - return true; } protected Thread newThread(Runnable runnable) @@ -675,17 +683,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 = 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]", getClass().getSimpleName(), _name, hashCode(), getState(), getMinThreads(), - getThreads(), + threads, getMaxThreads(), - getIdleThreads(), + starting, + idle, getReservedThreads(), - _jobs.size(), + queue, _tryExecutor); } @@ -760,19 +775,22 @@ 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,idle?1:0); // threads, starting, idle + + if (LOG.isDebugEnabled()) + LOG.debug("Runner started with {} for {}", job, QueuedThreadPool.this); while (true) { @@ -781,15 +799,26 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP if (!idle) { idle = true; - _threadsIdle.incrementAndGet(); + _counts.add(0,0,1); // threads, starting, idle } - job = idleJobPoll(); - if (job == SHRINK) + long idleTimeout = getIdleTimeout(); + job = idleJobPoll(idleTimeout); + + // maybe we should shrink? + if (job == null && getThreads() > _minThreads && idleTimeout > 0) { - if (LOG.isDebugEnabled()) - LOG.debug("shrinking {}", this); - break; + 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; + } + } } } @@ -799,15 +828,14 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP if (idle) { idle = false; - if (_threadsIdle.decrementAndGet() == 0) - startThreads(1); + _counts.add(0,0,-1); // threads, starting, idle } 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,45 +849,30 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP } catch (InterruptedException e) { + if (LOG.isDebugEnabled()) + LOG.debug("interrupted {} in {}", job, QueuedThreadPool.this); LOG.ignore(e); } 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,idle?-1:0); // threads, starting, idle removeThread(Thread.currentThread()); + ensureThreads(); - 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); + if (LOG.isDebugEnabled()) + LOG.debug("Runner exited for {}", QueuedThreadPool.this); } } - private Runnable idleJobPoll() throws InterruptedException + private Runnable idleJobPoll(long idleTimeout) throws InterruptedException { - if (_idleTimeout <= 0) + 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); + return _jobs.poll(idleTimeout, TimeUnit.MILLISECONDS); } } } 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)); + } + +} 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));