From 833f291975ef4ca5df18c6e5e4360a70583dac4f Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 1 Jun 2016 12:41:04 +1000 Subject: [PATCH 1/2] Ensure sessions that fail to save on evict are not evicted. --- .../server/session/AbstractSessionCache.java | 8 +- .../eclipse/jetty/server/session/Session.java | 4 +- .../server/session/EvictionFailureTest.java | 39 +++ .../AbstractSessionEvictionFailureTest.java | 256 ++++++++++++++++++ 4 files changed, 299 insertions(+), 8 deletions(-) create mode 100644 tests/test-sessions/test-hash-sessions/src/test/java/org/eclipse/jetty/server/session/EvictionFailureTest.java create mode 100644 tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractSessionEvictionFailureTest.java diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java index 32c31f1e8b5..9633ce1eae4 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java @@ -637,6 +637,7 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements if (session == null) return; + if (LOG.isDebugEnabled()) LOG.debug("Checking for idle {}", session.getId()); try (Lock s = session.lock()) { if (getEvictionPolicy() > 0 && session.isIdleLongerThan(getEvictionPolicy()) && session.isValid() && session.isResident() && session.getRequests() <= 0) @@ -656,9 +657,6 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements _sessionDataStore.store(session.getId(), session.getSessionData()); } - - //evict - // session.stopInactivityTimer(); doDelete(session.getId()); //detach from this cache session.setResident(false); @@ -666,9 +664,7 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements catch (Exception e) { LOG.warn("Passivation of idle session {} failed", session.getId(), e); - //TODO reset the timer so it can be retried later and leave it in the cache - doDelete(session.getId()); //detach it - session.setResident(false); + session.updateInactivityTimer(); } } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java index aa6a687bf6c..919edb7c394 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java @@ -259,7 +259,7 @@ public class Session implements SessionHandler.SessionIf long now = System.currentTimeMillis(); try (Lock lock = _lock.lockIfNotHeld()) { - return ((_sessionData.getAccessed() + (sec*1000)) < now); + return ((_sessionData.getAccessed() + (sec*1000)) <= now); } } @@ -485,7 +485,7 @@ public class Session implements SessionHandler.SessionIf { //we do not want to evict inactive sessions setInactivityTimer(-1L); - if (LOG.isDebugEnabled()) LOG.debug("Session is immortal && bo inactivity eviction: timer cancelled"); + if (LOG.isDebugEnabled()) LOG.debug("Session is immortal && no inactivity eviction: timer cancelled"); } else { diff --git a/tests/test-sessions/test-hash-sessions/src/test/java/org/eclipse/jetty/server/session/EvictionFailureTest.java b/tests/test-sessions/test-hash-sessions/src/test/java/org/eclipse/jetty/server/session/EvictionFailureTest.java new file mode 100644 index 00000000000..dbcbd52bf58 --- /dev/null +++ b/tests/test-sessions/test-hash-sessions/src/test/java/org/eclipse/jetty/server/session/EvictionFailureTest.java @@ -0,0 +1,39 @@ +// +// ======================================================================== +// Copyright (c) 1995-2016 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.server.session; + +/** + * EvictionFailureTest + * + * + */ +public class EvictionFailureTest extends AbstractSessionEvictionFailureTest +{ + + /** + * @see org.eclipse.jetty.server.session.AbstractTestBase#createServer(int, int, int, int) + */ + @Override + public AbstractTestServer createServer(int port, int maxInactive, int scavengeInterval, int evictionPolicy) + { + return new HashTestServer(port, maxInactive, scavengeInterval, evictionPolicy); + } + +} diff --git a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractSessionEvictionFailureTest.java b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractSessionEvictionFailureTest.java new file mode 100644 index 00000000000..81ae7e285c2 --- /dev/null +++ b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractSessionEvictionFailureTest.java @@ -0,0 +1,256 @@ +// +// ======================================================================== +// Copyright (c) 1995-2016 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.server.session; + + + + + +import java.io.IOException; +import java.util.Set; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.api.Request; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.junit.Test; + +/** + * AbstractSessionEvictionFailureTest + * + * + */ +public abstract class AbstractSessionEvictionFailureTest extends AbstractTestBase +{ + + /** + * TestSessionDataStore + * + * + */ + public static class TestSessionDataStore extends AbstractSessionDataStore + { + public boolean _nextResult; + + public void setNextResult (boolean goodOrBad) + { + _nextResult = goodOrBad; + } + + /** + * @see org.eclipse.jetty.server.session.SessionDataStore#isPassivating() + */ + @Override + public boolean isPassivating() + { + return false; + } + + /** + * @see org.eclipse.jetty.server.session.SessionDataStore#exists(java.lang.String) + */ + @Override + public boolean exists(String id) throws Exception + { + return true; + } + + /** + * @see org.eclipse.jetty.server.session.SessionDataMap#load(java.lang.String) + */ + @Override + public SessionData load(String id) throws Exception + { + return null; + } + + /** + * @see org.eclipse.jetty.server.session.SessionDataMap#delete(java.lang.String) + */ + @Override + public boolean delete(String id) throws Exception + { + return false; + } + + /** + * @see org.eclipse.jetty.server.session.AbstractSessionDataStore#doStore(java.lang.String, org.eclipse.jetty.server.session.SessionData, long) + */ + @Override + public void doStore(String id, SessionData data, long lastSaveTime) throws Exception + { + if (!_nextResult) + { + System.err.println("Throwing exception on store!"); + + throw new IllegalStateException("Testing store"); + } + else + System.err.println("Storing"); + + } + + /** + * @see org.eclipse.jetty.server.session.AbstractSessionDataStore#doGetExpired(java.util.Set) + */ + @Override + public Set doGetExpired(Set candidates) + { + return candidates; + } + } + + + + /** + * TestServlet + * + * + */ + public static class TestServlet extends HttpServlet + { + + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse httpServletResponse) throws ServletException, IOException + { + String action = request.getParameter("action"); + if ("init".equals(action)) + { + HttpSession session = request.getSession(true); + session.setAttribute("aaa", new Integer(0)); + } + else if ("test".equals(action)) + { + HttpSession session = request.getSession(false); + assertNotNull(session); + Integer count = (Integer)session.getAttribute("aaa"); + assertNotNull(count); + session.setAttribute("aaa", new Integer(count.intValue()+1)); + } + } + } + + + /** + * @param sec + */ + public static void pause (int sec) + { + try + { + Thread.currentThread().sleep(sec*1000L); + } + catch (InterruptedException e) + { + //just return; + } + } + + + @Test + public void testEvictFailure () throws Exception + { + String contextPath = ""; + String servletMapping = "/server"; + int inactivePeriod = 0; + int scavengePeriod = 20; + int evictionPeriod = 5; //evict after inactivity + AbstractTestServer server = createServer(0, inactivePeriod, scavengePeriod, evictionPeriod); + ServletContextHandler context = server.addContext(contextPath); + context.getSessionHandler().getSessionCache().setSaveOnInactiveEviction(true); + TestSessionDataStore ds = new TestSessionDataStore(); + context.getSessionHandler().getSessionCache().setSessionDataStore(ds); + + TestServlet servlet = new TestServlet(); + ServletHolder holder = new ServletHolder(servlet); + context.addServlet(holder, servletMapping); + + try + { + server.start(); + int port1 = server.getPort(); + HttpClient client = new HttpClient(); + client.start(); + try + { + String url = "http://localhost:" + port1 + contextPath + servletMapping; + + // Create the session + ds.setNextResult(true); + ContentResponse response = client.GET(url + "?action=init"); + assertEquals(HttpServletResponse.SC_OK,response.getStatus()); + String sessionCookie = response.getHeaders().get("Set-Cookie"); + assertNotNull(sessionCookie); + // Mangle the cookie, replacing Path with $Path, etc. + sessionCookie = sessionCookie.replaceFirst("(\\W)(P|p)ath=", "$1\\$Path="); + + ds.setNextResult(false); + + //Wait for the eviction period to expire - save on evict should fail but session + //should remain in the cache + pause(evictionPeriod); + + ds.setNextResult(true); + + // Make another request to see if the session is still in the cache and can be used, + //allow it to be saved this time + Request request = client.newRequest(url + "?action=test"); + request.header("Cookie", sessionCookie); + response = request.send(); + assertEquals(HttpServletResponse.SC_OK,response.getStatus()); + assertNull(response.getHeaders().get("Set-Cookie")); //check that the cookie wasn't reset + ds.setNextResult(false); + + System.err.println("Waiting again for session to expire"); + + //Wait for the eviction period to expire again + pause(evictionPeriod); + + ds.setNextResult(true); + + request = client.newRequest(url + "?action=test"); + request.header("Cookie", sessionCookie); + response = request.send(); + assertEquals(HttpServletResponse.SC_OK,response.getStatus()); + } + finally + { + client.stop(); + } + } + finally + { + server.stop(); + } + } + +} From c2831bf09ccc346436157f34452d7addc82d7870 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 1 Jun 2016 15:28:04 +1000 Subject: [PATCH 2/2] Issue #608 reset encoding set from content type Use an enum to track where a content encoding came from and selectively clear/reset --- .../org/eclipse/jetty/server/Response.java | 49 ++++++++++++++----- .../eclipse/jetty/server/ResponseTest.java | 34 ++++++++++++- 2 files changed, 69 insertions(+), 14 deletions(-) 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 a119190a752..ee7f42aa795 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -24,6 +24,7 @@ import java.nio.channels.IllegalSelectorException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.EnumSet; import java.util.Enumeration; import java.util.Iterator; import java.util.Locale; @@ -108,12 +109,15 @@ public class Response implements HttpServletResponse private Locale _locale; private MimeTypes.Type _mimeType; private String _characterEncoding; - private boolean _explicitEncoding; + private EncodingFrom _encodingFrom=EncodingFrom.NOT_SET; private String _contentType; private OutputType _outputType = OutputType.NONE; private ResponseWriter _writer; private long _contentLength = -1; + private enum EncodingFrom { NOT_SET, INFERRED, SET_LOCALE, SET_CONTENT_TYPE, SET_CHARACTER_ENCODING }; + private static final EnumSet __localeOverride = EnumSet.of(EncodingFrom.NOT_SET,EncodingFrom.INFERRED); + public Response(HttpChannel channel, HttpOutput out) { @@ -138,7 +142,7 @@ public class Response implements HttpServletResponse _contentLength = -1; _out.recycle(); _fields.clear(); - _explicitEncoding=false; + _encodingFrom=EncodingFrom.NOT_SET; } public HttpOutput getHttpOutput() @@ -899,7 +903,7 @@ public class Response implements HttpServletResponse encoding = MimeTypes.inferCharsetFromContentType(_contentType); if (encoding == null) encoding = StringUtil.__ISO_8859_1; - setCharacterEncoding(encoding,false); + setCharacterEncoding(encoding,EncodingFrom.INFERRED); } } @@ -1015,10 +1019,10 @@ public class Response implements HttpServletResponse @Override public void setCharacterEncoding(String encoding) { - setCharacterEncoding(encoding,true); + setCharacterEncoding(encoding,EncodingFrom.SET_CHARACTER_ENCODING); } - private void setCharacterEncoding(String encoding, boolean explicit) + private void setCharacterEncoding(String encoding, EncodingFrom from) { if (isIncluding() || isWriting()) return; @@ -1027,7 +1031,7 @@ public class Response implements HttpServletResponse { if (encoding == null) { - _explicitEncoding=false; + _encodingFrom=EncodingFrom.NOT_SET; // Clear any encoding. if (_characterEncoding != null) @@ -1050,7 +1054,7 @@ public class Response implements HttpServletResponse else { // No, so just add this one to the mimetype - _explicitEncoding = explicit; + _encodingFrom = from; _characterEncoding = HttpGenerator.__STRICT?encoding:StringUtil.normalizeCharset(encoding); if (_mimeType!=null) { @@ -1100,10 +1104,29 @@ public class Response implements HttpServletResponse if (charset == null) { - if (_characterEncoding != null) + switch (_encodingFrom) { - _contentType = contentType + ";charset=" + _characterEncoding; - _mimeType = null; + case NOT_SET: + break; + case INFERRED: + case SET_CONTENT_TYPE: + if (isWriting()) + { + _mimeType=null; + _contentType = _contentType + ";charset=" + _characterEncoding; + } + else + { + _encodingFrom=EncodingFrom.NOT_SET; + _characterEncoding=null; + } + break; + case SET_LOCALE: + case SET_CHARACTER_ENCODING: + { + _contentType = contentType + ";charset=" + _characterEncoding; + _mimeType = null; + } } } else if (isWriting() && !charset.equalsIgnoreCase(_characterEncoding)) @@ -1117,7 +1140,7 @@ public class Response implements HttpServletResponse else { _characterEncoding = charset; - _explicitEncoding = true; + _encodingFrom = EncodingFrom.SET_CONTENT_TYPE; } if (HttpGenerator.__STRICT || _mimeType==null) @@ -1268,8 +1291,8 @@ public class Response implements HttpServletResponse String charset = _channel.getRequest().getContext().getContextHandler().getLocaleEncoding(locale); - if (charset != null && charset.length() > 0 && !_explicitEncoding) - setCharacterEncoding(charset,false); + if (charset != null && charset.length() > 0 && __localeOverride.contains(_encodingFrom)) + setCharacterEncoding(charset,EncodingFrom.SET_LOCALE); } @Override 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 fc7d19e0e58..58500fc4dd4 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 @@ -165,7 +165,7 @@ public class ResponseTest assertEquals("text/html", response.getContentType()); response.getWriter(); assertEquals("text/html;charset=utf-8", response.getContentType()); - response.setContentType("foo2/bar2"); + response.setContentType("foo2/bar2;charset=utf-8"); assertEquals("foo2/bar2;charset=utf-8", response.getContentType()); response.recycle(); @@ -357,6 +357,38 @@ public class ResponseTest assertEquals("text/xml;charset=utf-8", response.getContentType()); } + @Test + public void testResetContentTypeWithoutCharacterEncoding() throws Exception + { + Response response = newResponse(); + + response.setCharacterEncoding("utf-8"); + response.setContentType("wrong/answer"); + response.setContentType("foo/bar"); + assertEquals("foo/bar;charset=utf-8", response.getContentType()); + response.getWriter(); + response.setContentType("foo2/bar2"); + assertEquals("foo2/bar2;charset=utf-8", response.getContentType()); + } + + + @Test + public void testResetContentTypeWithCharacterEncoding() throws Exception + { + Response response = newResponse(); + + response.setContentType("wrong/answer;charset=utf-8"); + response.setContentType("foo/bar"); + assertEquals("foo/bar", response.getContentType()); + response.setContentType("wrong/answer;charset=utf-8"); + response.getWriter(); + response.setContentType("foo2/bar2;charset=utf-16"); + assertEquals("foo2/bar2;charset=utf-8", response.getContentType()); + } + + + + @Test public void testContentTypeWithOther() throws Exception {