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.

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.

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.

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());
}

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?

Created attachment 36521 [details]
patch for master

adding new patch for PR 154

Created attachment 36522 [details]
patch for 8.5.x

adding patch for 8.5.x (PR 155)

Created attachment 36523 [details]
patch for 7.0.x (PR 156)

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