ASF Bugzilla – Attachment 36522 Details for
Bug 63324
CrawlerSessionManagerValve is getting put into Session, which causes problems when serializing sessions
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
patch for 8.5.x
155-8.5.x.patch (text/plain), 13.30 KB, created by
Martin L
on 2019-04-12 08:22:24 UTC
(
hide
)
Description:
patch for 8.5.x
Filename:
MIME Type:
Creator:
Martin L
Created:
2019-04-12 08:22:24 UTC
Size:
13.30 KB
patch
obsolete
>From c3a839d20a1988974e5e797866c1b562f84b8a16 Mon Sep 17 00:00:00 2001 >From: Martin Lemanski <martin.lemanski@willhaben.at> >Date: Mon, 8 Apr 2019 15:42:08 +0200 >Subject: [PATCH 1/3] 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 aab1caf227..4c48310cfe 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<String, String> clientIdSessionId; >+ private final Map<String, String> sessionIdClientId; > >- @Override >- public void valueBound(HttpSessionBindingEvent event) { >- // NOOP >- } >+ public CrawlerHttpSessionBindingListener(Map<String, String> clientIdSessionId, Map<String, String> 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 759a248d1f..847ad2f37e 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 453ccd8aa7b1b5c94407ab6217cfa9fc9aaef9f0 Mon Sep 17 00:00:00 2001 >From: Martin Lemanski <martin.lemanski@willhaben.at> >Date: Tue, 9 Apr 2019 14:50:34 +0200 >Subject: [PATCH 2/3] 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 | 17 +++++++++-------- > .../valves/TestCrawlerSessionManagerValve.java | 1 - > 2 files changed, 9 insertions(+), 9 deletions(-) > >diff --git a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java >index 4c48310cfe..e5ac0be3fc 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<String, String> clientIdSessionId; >- private final Map<String, String> sessionIdClientId; >+ private static class CrawlerHttpSessionBindingListener implements HttpSessionBindingListener, Serializable { >+ private final transient Map<String, String> clientIdSessionId; >+ private final transient String clientIdentifier; > >- public CrawlerHttpSessionBindingListener(Map<String, String> clientIdSessionId, Map<String, String> sessionIdClientId) { >+ private CrawlerHttpSessionBindingListener(Map<String, String> clientIdSessionId, String clientIdentifier) { > this.clientIdSessionId = clientIdSessionId; >- this.sessionIdClientId = sessionIdClientId; >+ this.clientIdentifier = clientIdentifier; > } > > @Override >@@ -285,8 +286,8 @@ public void valueBound(HttpSessionBindingEvent event) { > > @Override > public void valueUnbound(HttpSessionBindingEvent event) { >- String clientIdentifier = sessionIdClientId.remove(event.getSession().getId()); >- if (clientIdentifier != null) { >+ if (clientIdentifier != null && clientIdSessionId != null) { >+ if(clientIdSessionId.get(clientIdentifier).equals(event.getSession().getId())) > clientIdSessionId.remove(clientIdentifier); > } > } >diff --git a/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java b/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java >index 847ad2f37e..aef1317052 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; > >From 2448177c3fcc0b2c8025f219d9309e4eef24e80a Mon Sep 17 00:00:00 2001 >From: Martin Lemanski <martin.lemanski@willhaben.at> >Date: Tue, 9 Apr 2019 14:56:14 +0200 >Subject: [PATCH 3/3] adds braces to if statement > >can't remove with `Map.remove(key, value)`, compiler complains about types > >fix 63324 >--- > .../catalina/valves/CrawlerSessionManagerValve.java | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > >diff --git a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java >index e5ac0be3fc..6311877be2 100644 >--- a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java >+++ b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java >@@ -88,8 +88,8 @@ public void setCrawlerUserAgents(String crawlerUserAgents) { > } > > /** >+ * @return The current regular expression being used to match user agents. > * @see #setCrawlerUserAgents(String) >- * @return The current regular expression being used to match user agents. > */ > public String getCrawlerUserAgents() { > return crawlerUserAgents; >@@ -113,8 +113,8 @@ public void setCrawlerIps(String crawlerIps) { > } > > /** >- * @see #setCrawlerIps(String) > * @return The current regular expression being used to match IP addresses. >+ * @see #setCrawlerIps(String) > */ > public String getCrawlerIps() { > return crawlerIps; >@@ -125,15 +125,15 @@ public String getCrawlerIps() { > * Specify the session timeout (in seconds) for a crawler's session. This is > * typically lower than that for a user session. The default is 60 seconds. > * >- * @param sessionInactiveInterval The new timeout for crawler sessions >+ * @param sessionInactiveInterval The new timeout for crawler sessions > */ > public void setSessionInactiveInterval(int sessionInactiveInterval) { > this.sessionInactiveInterval = sessionInactiveInterval; > } > > /** >+ * @return The current timeout in seconds > * @see #setSessionInactiveInterval(int) >- * @return The current timeout in seconds > */ > public int getSessionInactiveInterval() { > return sessionInactiveInterval; >@@ -287,8 +287,9 @@ public void valueBound(HttpSessionBindingEvent event) { > @Override > public void valueUnbound(HttpSessionBindingEvent event) { > if (clientIdentifier != null && clientIdSessionId != null) { >- if(clientIdSessionId.get(clientIdentifier).equals(event.getSession().getId())) >- clientIdSessionId.remove(clientIdentifier); >+ if (clientIdSessionId.get(clientIdentifier).equals(event.getSession().getId())) { >+ clientIdSessionId.remove(clientIdentifier); >+ } > } > } > }
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
Attachments on
bug 63324
:
36512
|
36515
|
36521
| 36522 |
36523