Bug 63324 - CrawlerSessionManagerValve is getting put into Session, which causes problems when serializing sessions
Summary: CrawlerSessionManagerValve is getting put into Session, which causes problems...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.x-trunk
Hardware: PC Linux
: P2 minor (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-08 14:37 UTC by Martin L
Modified: 2019-05-01 10:56 UTC (History)
1 user (show)



Attachments
the stacktrace, which indicates the serialization (11.28 KB, text/plain)
2019-04-08 14:37 UTC, Martin L
Details
154.patch (a copy of PR 154) (6.37 KB, patch)
2019-04-09 09:21 UTC, Konstantin Kolinko
Details | Diff
patch for master (10.22 KB, patch)
2019-04-12 08:21 UTC, Martin L
Details | Diff
patch for 8.5.x (13.30 KB, patch)
2019-04-12 08:22 UTC, Martin L
Details | Diff
patch for 7.0.x (PR 156) (10.73 KB, patch)
2019-04-12 08:23 UTC, Martin L
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin L 2019-04-08 14:37:27 UTC
Created attachment 36512 [details]
the stacktrace, which indicates the serialization

We are using a serialization library to serialize our sessions between multiple tomcat instances. 

During our OpenJDK 11 upgrade, we've found out, that the CrawlerSessionManagerValve is in the session, which has dependencies to JDK internals.

We don't want to serialize JDK internals between server ups and downs. 

I've already created a PR to fix this issue, by providing a smaller class to the session attributes. 

https://github.com/apache/tomcat/pull/154
Comment 1 Martin L 2019-04-08 14:46:06 UTC
For a more broad understanding:

We have a mem-cached session serialization strategy for our multiple tomcat instances. When one server goes down, we serialize the sessions into memcache and put it on a different server which is still running.
Comment 2 Konstantin Kolinko 2019-04-09 09:21:46 UTC
Created attachment 36515 [details]
154.patch (a copy of PR 154)

A copy of the current code in PR 154, formatted as a patch.

I'll comment on it, so let's keep a copy here.
Comment 3 Konstantin Kolinko 2019-04-09 09:22:25 UTC
Note that the assumption of CrawlerSessionManagerValve is that the clients does not support cookies. Thus it forcefully assigns them to the same session based on their IP addresses.

[[[
            if (isBot) {
                sessionId = clientIdSessionId.get(clientIdentifier);
]]]


1) If the client really does not support cookies, once you stop and start Tomcat, the "clientIdSessionId" map is lost and you have lost access to those sessions. They will never be accessed again.

They will just time out after some time elapses - "sessionInactiveInterval" in CrawlerSessionManagerValve defaults to 60 seconds.

(Thus it makes sense to do not serialize those sessions at all, to do not replicate them etc.

Serializing the original maps (like proposed by PR 154) does not make any sense.)

2) If client supports cookies, you do not need a CrawlerSessionManagerValve.

Thus you not not need a value in a "clientIdSessionId" map. You will access the session using the sessionid provided by a Cookie.
Comment 4 Konstantin Kolinko 2019-04-09 09:34:46 UTC
Based on the analysis in comment #3 I think that a possible solution is to adjust the original proposal as follows. I am quoting fragments from attachment 36515 [details]

[[[
+    private static class CrawlerHttpSessionBindingListener implements HttpSessionBindingListener {
+        private final Map<String, String> clientIdSessionId;
+        private final Map<String, String> sessionIdClientId;
]]]

Three changes are needed:

1. Declare CrawlerHttpSessionBindingListener to implement java.io.Serializable.

2. The "clientIdSessionId" field should be declared transient.

3. The "sessionIdClientId" field does not need to be a Map. Just add a String field "String clientIdentifier" that stores a single value. The field can be transient as well.

[[[
+        @Override
+        public void valueUnbound(HttpSessionBindingEvent event) {
+            String clientIdentifier = sessionIdClientId.remove(event.getSession().getId());
+            if (clientIdentifier != null) {
+                clientIdSessionId.remove(clientIdentifier);
+            }
         }
]]]

4. Add a check that clientIdSessionId is not null.

Duplicate removals can he handled by using method Map.remove(key, value) that removes a key only if the value matches.

if (clientIdentifier != null && clientIdSessionId != null) {
   clientIdSessionId.remove(clientIdentifier, event.getSession().getId());
}
Comment 5 Martin L 2019-04-09 13:04:13 UTC
Thanks for your feedback.

I've applied your suggestions, but needed to customize them a bit, depending on the branch.
There are 3 PRs, one for 7.0.x, one for 8.5.x and one for master.

In 7.0.x the test setup looks a slightly bit different, and you need to implement both interface methods of HttpSessionBindingListener
https://github.com/apache/tomcat/pull/156


In 8.5.x I couldn't use the signature Map.remove(key, value). 
I was getting a `method remove in interface Map<K,V> cannot be applied to given types; clientIdSessionId.remove(clientIdentifier, event.getSession().getId());` when running the tests. 
So I needed to check for the same value in an extra if.
https://github.com/apache/tomcat/pull/155

In master is the "cleanest" solution.
https://github.com/apache/tomcat/pull/154

Do you want me to upload all 3 patches here?
Comment 6 Martin L 2019-04-12 08:21:16 UTC
Created attachment 36521 [details]
patch for master

adding new patch for PR 154
Comment 7 Martin L 2019-04-12 08:22:24 UTC
Created attachment 36522 [details]
patch for 8.5.x

adding patch for 8.5.x (PR 155)
Comment 8 Martin L 2019-04-12 08:23:16 UTC
Created attachment 36523 [details]
patch for 7.0.x (PR 156)
Comment 9 Mark Thomas 2019-05-01 10:56:34 UTC
Thanks for the report and the patches to fix this issue. Updating the test cases is particularly appreciated.

Fixed in:
- master for 9.0.20 onwards
- 8.5.x for 8.5.41 onwards
- 7.0.x for 7.0.95 onwards