From 03c30ee571c2d82aba33c2219961ec28542d5c6d Mon Sep 17 00:00:00 2001 From: Martin Lemanski Date: Mon, 8 Apr 2019 15:42:08 +0200 Subject: [PATCH 1/2] Don't serialize the whole valve into session, but use a smaller object. It helps to serialize this sessions with libraries like kryo. Background: we've found out, that our kryo library serializes the whole valve. Our stack indicates, that jdk internals are getting serialized into the sessions. It's unnecessary and can be solves more easily with this fix. --- .../valves/CrawlerSessionManagerValve.java | 30 +++++++----- .../TestCrawlerSessionManagerValve.java | 46 ++++++++++++++++++- 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java index ac35a3b262..659a3ab724 100644 --- a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java +++ b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java @@ -42,7 +42,7 @@ * users - regardless of whether or not they provide a session token with their * requests. */ -public class CrawlerSessionManagerValve extends ValveBase implements HttpSessionBindingListener { +public class CrawlerSessionManagerValve extends ValveBase { private static final Log log = LogFactory.getLog(CrawlerSessionManagerValve.class); @@ -241,7 +241,7 @@ public void invoke(Request request, Response response) throws IOException, Servl clientIdSessionId.put(clientIdentifier, s.getId()); sessionIdClientId.put(s.getId(), clientIdentifier); // #valueUnbound() will be called on session expiration - s.setAttribute(this.getClass().getName(), this); + s.setAttribute(this.getClass().getName(), new CrawlerHttpSessionBindingListener(clientIdSessionId, sessionIdClientId)); s.setMaxInactiveInterval(sessionInactiveInterval); if (log.isDebugEnabled()) { @@ -269,18 +269,26 @@ private String getClientIdentifier(Host host, Context context, String clientIp) return result.toString(); } + private static class CrawlerHttpSessionBindingListener implements HttpSessionBindingListener { + private final Map clientIdSessionId; + private final Map sessionIdClientId; - @Override - public void valueBound(HttpSessionBindingEvent event) { - // NOOP - } + public CrawlerHttpSessionBindingListener(Map clientIdSessionId, Map sessionIdClientId) { + this.clientIdSessionId = clientIdSessionId; + this.sessionIdClientId = sessionIdClientId; + } + @Override + public void valueBound(HttpSessionBindingEvent event) { + // NOOP + } - @Override - public void valueUnbound(HttpSessionBindingEvent event) { - String clientIdentifier = sessionIdClientId.remove(event.getSession().getId()); - if (clientIdentifier != null) { - clientIdSessionId.remove(clientIdentifier); + @Override + public void valueUnbound(HttpSessionBindingEvent event) { + String clientIdentifier = sessionIdClientId.remove(event.getSession().getId()); + if (clientIdentifier != null) { + clientIdSessionId.remove(clientIdentifier); + } } } } diff --git a/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java b/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java index 59a192ce42..a4203f1561 100644 --- a/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java +++ b/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java @@ -22,7 +22,13 @@ import javax.servlet.ServletException; import javax.servlet.http.HttpSession; +import javax.servlet.http.HttpSessionBindingListener; +import org.apache.catalina.Manager; +import org.apache.catalina.core.StandardContext; +import org.apache.catalina.session.StandardManager; +import org.apache.catalina.session.StandardSession; +import org.hamcrest.CoreMatchers; import org.junit.Test; import org.apache.catalina.Context; @@ -33,8 +39,20 @@ import org.easymock.EasyMock; import org.easymock.IExpectationSetters; +import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.MatcherAssert.assertThat; + public class TestCrawlerSessionManagerValve { + private static final Manager TEST_MANAGER; + + static { + TEST_MANAGER = new StandardManager(); + TEST_MANAGER.setContext(new StandardContext()); + } + + + @Test public void testCrawlerIpsPositive() throws Exception { CrawlerSessionManagerValve valve = new CrawlerSessionManagerValve(); @@ -79,6 +97,32 @@ public void testCrawlerMultipleHostsHostAware() throws Exception { verifyCrawlingLocalhost(valve, "example.invalid"); } + @Test + public void testCrawlersSessionIdIsRemovedAfterSessionExpiry() throws IOException, ServletException { + CrawlerSessionManagerValve valve = new CrawlerSessionManagerValve(); + valve.setCrawlerIps("216\\.58\\.206\\.174"); + valve.setCrawlerUserAgents(valve.getCrawlerUserAgents()); + valve.setNext(EasyMock.createMock(Valve.class)); + valve.setSessionInactiveInterval(0); + StandardSession session = new StandardSession(TEST_MANAGER); + session.setId("id"); + session.setValid(true); + + Request request = createRequestExpectations("216.58.206.174", session, true); + + EasyMock.replay(request); + + valve.invoke(request, EasyMock.createMock(Response.class)); + + EasyMock.verify(request); + + assertThat(valve.getClientIpSessionId().values(), hasItem("id")); + + session.expire(); + + assertThat(valve.getClientIpSessionId().values().size(), is(0)); + } + private void verifyCrawlingLocalhost(CrawlerSessionManagerValve valve, String hostname) throws IOException, ServletException { @@ -97,7 +141,7 @@ private HttpSession createSessionExpectations(CrawlerSessionManagerValve valve, HttpSession session = EasyMock.createMock(HttpSession.class); if (isBot) { EasyMock.expect(session.getId()).andReturn("id").times(2); - session.setAttribute(valve.getClass().getName(), valve); + session.setAttribute(EasyMock.eq(valve.getClass().getName()), EasyMock.anyObject(HttpSessionBindingListener.class)); EasyMock.expectLastCall(); session.setMaxInactiveInterval(60); EasyMock.expectLastCall(); From e5128bf506202398415ab41b74b4362026b2419a Mon Sep 17 00:00:00 2001 From: Martin Lemanski Date: Tue, 9 Apr 2019 14:43:35 +0200 Subject: [PATCH 2/2] Don't serialize the whole valve into session, but use a smaller object. It helps to serialize this sessions with libraries like kryo. Apply feedback from PR review --- .../valves/CrawlerSessionManagerValve.java | 18 +++++++++--------- .../valves/TestCrawlerSessionManagerValve.java | 3 +-- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java index 659a3ab724..49eb49cbe0 100644 --- a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java +++ b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java @@ -17,6 +17,7 @@ package org.apache.catalina.valves; import java.io.IOException; +import java.io.Serializable; import java.util.Enumeration; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -241,7 +242,7 @@ public void invoke(Request request, Response response) throws IOException, Servl clientIdSessionId.put(clientIdentifier, s.getId()); sessionIdClientId.put(s.getId(), clientIdentifier); // #valueUnbound() will be called on session expiration - s.setAttribute(this.getClass().getName(), new CrawlerHttpSessionBindingListener(clientIdSessionId, sessionIdClientId)); + s.setAttribute(this.getClass().getName(), new CrawlerHttpSessionBindingListener(clientIdSessionId, clientIdentifier)); s.setMaxInactiveInterval(sessionInactiveInterval); if (log.isDebugEnabled()) { @@ -269,13 +270,13 @@ private String getClientIdentifier(Host host, Context context, String clientIp) return result.toString(); } - private static class CrawlerHttpSessionBindingListener implements HttpSessionBindingListener { - private final Map clientIdSessionId; - private final Map sessionIdClientId; + private static class CrawlerHttpSessionBindingListener implements HttpSessionBindingListener, Serializable { + private final transient Map clientIdSessionId; + private final transient String clientIdentifier; - public CrawlerHttpSessionBindingListener(Map clientIdSessionId, Map sessionIdClientId) { + private CrawlerHttpSessionBindingListener(Map clientIdSessionId, String clientIdentifier) { this.clientIdSessionId = clientIdSessionId; - this.sessionIdClientId = sessionIdClientId; + this.clientIdentifier = clientIdentifier; } @Override @@ -285,9 +286,8 @@ public void valueBound(HttpSessionBindingEvent event) { @Override public void valueUnbound(HttpSessionBindingEvent event) { - String clientIdentifier = sessionIdClientId.remove(event.getSession().getId()); - if (clientIdentifier != null) { - clientIdSessionId.remove(clientIdentifier); + if (clientIdentifier != null && clientIdSessionId != null) { + clientIdSessionId.remove(clientIdentifier, event.getSession().getId()); } } } diff --git a/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java b/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java index a4203f1561..b1360a5eaf 100644 --- a/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java +++ b/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java @@ -28,7 +28,6 @@ import org.apache.catalina.core.StandardContext; import org.apache.catalina.session.StandardManager; import org.apache.catalina.session.StandardSession; -import org.hamcrest.CoreMatchers; import org.junit.Test; import org.apache.catalina.Context; @@ -48,7 +47,7 @@ static { TEST_MANAGER = new StandardManager(); - TEST_MANAGER.setContext(new StandardContext()); + TEST_MANAGER.setContainer(new StandardContext()); }