From 7cddc41b007527d1b4247e05619d8adf81374420 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 | 23 +++++++--- .../TestCrawlerSessionManagerValve.java | 46 ++++++++++++++++++- 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java index a268d4b426..fdf39577ab 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,12 +269,21 @@ 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 valueUnbound(HttpSessionBindingEvent event) { - String clientIdentifier = sessionIdClientId.remove(event.getSession().getId()); - if (clientIdentifier != null) { - clientIdSessionId.remove(clientIdentifier); + public CrawlerHttpSessionBindingListener(Map clientIdSessionId, Map sessionIdClientId) { + this.clientIdSessionId = clientIdSessionId; + this.sessionIdClientId = sessionIdClientId; + } + + @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 f7a7e2652d..9f0df1361e 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 579af295b5c3047699060dd969ad9b966551354e Mon Sep 17 00:00:00 2001 From: Martin Lemanski Date: Tue, 9 Apr 2019 14:53:45 +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 fix 63324 --- .../valves/CrawlerSessionManagerValve.java | 18 +++++++++--------- .../valves/TestCrawlerSessionManagerValve.java | 1 - 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java index fdf39577ab..d85f1537dd 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,20 +270,19 @@ 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 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 9f0df1361e..3f4b978ed4 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;