Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Received-SPF: pass (athena.apache.org: domain of nicholas@nicholaswilliams.net
 designates 209.85.223.170 as permitted sender)
From: Nick Williams <nicholas@nicholaswilliams.net>
Mime-Version: 1.0 (Apple Message framework v1283)
Content-Type: multipart/signed;
 boundary="Apple-Mail=_61241DAF-2484-4BAE-B0C5-6E1E4B5830CC";
 protocol="application/pkcs7-signature"; micalg=sha1
Subject: Re: svn commit: r1445517 - in /tomcat/trunk: java/javax/servlet/http/
 java/org/apache/catalina/connector/ java/org/apache/catalina/core/
 java/org/apache/catalina/ha/session/ java/org/apache/catalina/session/
 java/org/apache/catalina/websocket/ webapps/...
Date: Wed, 13 Feb 2013 11:05:33 -0600
In-Reply-To: 
 <CAHHaTeG+j9Rkh=2U3gPLQr6wyf6bs1fN3aQXb_y=L2VjfQwtmQ@mail.gmail.com>
To: Tomcat Developers List <dev@tomcat.apache.org>
References: <20130213092900.27A7723889CB@eris.apache.org>
 <CAHHaTeG+j9Rkh=2U3gPLQr6wyf6bs1fN3aQXb_y=L2VjfQwtmQ@mail.gmail.com>
Message-Id: <82AA0C3C-ACF4-4F55-8E01-D3C3291DCEBC@nicholaswilliams.net>

--Apple-Mail=_61241DAF-2484-4BAE-B0C5-6E1E4B5830CC
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=windows-1252


On Feb 13, 2013, at 4:26 AM, Keiichi Fujino wrote:

> 2013/2/13  <markt@apache.org>:
>> Author: markt
>> Date: Wed Feb 13 09:28:58 2013
>> New Revision: 1445517
>>=20
>> URL: http://svn.apache.org/r1445517
>> Log:
>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=3D54552
>> Servlet 3.1
>> Implement HttpSessionIdListener and =
HttpServletRequest#changeSessionId()
>> Patch provided by Nick Williams.
>>=20
> I think that this code itself is not a problem at all. (I think prety =
good.)

Thanks! I was a bit nervous about my first code contribution. :-)

> However I have a idea of code improvement.
>=20
> There seems to be a duplicate code on ManagerBase and DeltaManager.

I struggled with that, too. There's a tellNew() method in =
StandardSession that is responsible for firing the =
Session.SESSION_CREATED_EVENT event and notifying HttpSessionListeners, =
and I thought about implementing a similar tellChangedSessionId method =
in StandardSession. However, since the code for firing the =
Context.CHANGE_SESSION_ID_EVENT internal event was also duplicated in =
ManagerBase and DeltaManager, I decided to stick with doing the same and =
keeping the code alongside those event firings. I would not disagree =
that the code should be centralized=85in both cases.

> e.g.
> By introducing a following method to ManagerBase and DeltaManager,
> we might be able to avoid code duplication.
>=20
> =3D=3D
> changeSessionId(Session session, String newId)
> changeSessionId(Session session, String newId,
>    boolean notifySessionListeners, boolean notifyContainerListeners)
> =3D=3D

If you are going to add a method to prevent code duplication, I would =
recommend it be something like =
StandardSession.tellChangedSessionId(String, String, boolean, boolean) =
(perhaps right below the tellNew() method) instead of in the ManagerBase =
class.

>=20
> And furthermore, we are changing sessionId in JvmRouteBinderValve.
> Change sessionid of JvmRouteBinderValve is completely different from
> Manager#changeSessionId.
> By using new changeSessionId method, will be able to change sessionId
> in a same way.
> As a result, JvmRouteSessionIDBinderListener will be unnecessary.

I took a look at JvmRouteSessionIDBinderListener and I do not believe it =
is changing the session ID in the same way (though I could certainly be =
wrong). It looks to me like it is involved in binding the JVMRoute to =
the end of the session ID in order to support clustering? I do not think =
that is the same thing as this.

Other areas in the code that "change" the session ID =
(changeSessionIdOnAuthentication, change replicated from another =
cluster, HttpServletRequest.changeSessionId()) call the =
changeSessionId(Session) method on the Manager, which changes the =
sessionId, does NOT call tellNew(), fires =
Context.CHANGE_SESSION_ID_EVENT and (now) notifies =
HttpSessionIdListeners. However, JvmRouteSessionIDBinderListener does it =
differently: it calls setId() directly on the session, DOES call =
tellNew() (is this right?) and bypasses the =
Context.CHANGE_SESSION_ID_EVENT and listeners by not calling =
Manager.changeSessionId(Session).

It could be that this was done this way intentionally, or not. I don't =
know. I'd like to understand it better. Someone more in-the-know than I =
can hopefully chime in.

>=20
> I'm going to fix these improvements If there is no objections from =
anyone.
> Any objections and comment?

If my code can be improved, I am always happy for someone to do it!

Nick


--Apple-Mail=_61241DAF-2484-4BAE-B0C5-6E1E4B5830CC
Content-Disposition: attachment;
	filename=smime.p7s
Content-Type: application/pkcs7-signature;
	name=smime.p7s
Content-Transfer-Encoding: base64

MIAGCSqGSIb3DQEHAqCAMIACAQExCzAJBgUrDgMCGgUAMIAGCSqGSIb3DQEHAQAAoIINcTCCBjQw
ggQcoAMCAQICAR4wDQYJKoZIhvcNAQEFBQAwfTELMAkGA1UEBhMCSUwxFjAUBgNVBAoTDVN0YXJ0
Q29tIEx0ZC4xKzApBgNVBAsTIlNlY3VyZSBEaWdpdGFsIENlcnRpZmljYXRlIFNpZ25pbmcxKTAn
BgNVBAMTIFN0YXJ0Q29tIENlcnRpZmljYXRpb24gQXV0aG9yaXR5MB4XDTA3MTAyNDIxMDE1NVoX
DTE3MTAyNDIxMDE1NVowgYwxCzAJBgNVBAYTAklMMRYwFAYDVQQKEw1TdGFydENvbSBMdGQuMSsw
KQYDVQQLEyJTZWN1cmUgRGlnaXRhbCBDZXJ0aWZpY2F0ZSBTaWduaW5nMTgwNgYDVQQDEy9TdGFy
dENvbSBDbGFzcyAxIFByaW1hcnkgSW50ZXJtZWRpYXRlIENsaWVudCBDQTCCASIwDQYJKoZIhvcN
AQEBBQADggEPADCCAQoCggEBAMcJg8zOLdgasSmkLhOrlr6KMoOMpohBllVHrdRvEg/q6r8jR+EK
75xCGhR8ToREoqe7zM9/UnC6TS2y9UKTpT1v7RSMzR0t6ndl0TWBuUr/UXBhPk+Kmy7bI4yW4urC
+y7P3/1/X7U8ocb8VpH/Clt+4iq7nirMcNh6qJR+xjOhV+VHzQMALuGYn5KZmc1NbJQYclsGkDxD
z2UbFqE2+6vIZoL+jb9x4Pa5gNf1TwSDkOkikZB1xtB4ZqtXThaABSONdfmv/Z1pua3FYxnCFmdr
/+N2JLKutIxMYqQOJebr/f/h5t95m4JgrM3Y/w7YX9d7YAL9jvN4SydHsU6n65cCAwEAAaOCAa0w
ggGpMA8GA1UdEwEB/wQFMAMBAf8wDgYDVR0PAQH/BAQDAgEGMB0GA1UdDgQWBBRTcu2SnODaywFc
fH6WNU7y1LhRgjAfBgNVHSMEGDAWgBROC+8apEBbpRdphzDKNGhD0EGu8jBmBggrBgEFBQcBAQRa
MFgwJwYIKwYBBQUHMAGGG2h0dHA6Ly9vY3NwLnN0YXJ0c3NsLmNvbS9jYTAtBggrBgEFBQcwAoYh
aHR0cDovL3d3dy5zdGFydHNzbC5jb20vc2ZzY2EuY3J0MFsGA1UdHwRUMFIwJ6AloCOGIWh0dHA6
Ly93d3cuc3RhcnRzc2wuY29tL3Nmc2NhLmNybDAnoCWgI4YhaHR0cDovL2NybC5zdGFydHNzbC5j
b20vc2ZzY2EuY3JsMIGABgNVHSAEeTB3MHUGCysGAQQBgbU3AQIBMGYwLgYIKwYBBQUHAgEWImh0
dHA6Ly93d3cuc3RhcnRzc2wuY29tL3BvbGljeS5wZGYwNAYIKwYBBQUHAgEWKGh0dHA6Ly93d3cu
c3RhcnRzc2wuY29tL2ludGVybWVkaWF0ZS5wZGYwDQYJKoZIhvcNAQEFBQADggIBAAqDCH14qywG
XLhjjF6uHLkjd02hcdh9hrw+VUsv+q1eeQWB21jWj3kJ96AUlPCoEGZ/ynJNScWy6QMVQjbbMXlt
UfO4n4bGGdKo3awPWp61tjAFgraLJgDk+DsSvUD6EowjMTNx25GQgyYJ5RPIzKKR9tQW8gGK+2+R
HxkUCTbYFnL6kl8Ch507rUdPPipJ9CgJFws3kDS3gOS5WFMxcjO5DwKfKSETEPrHh7p5shuuNktv
sv6hxHTLhiMKX893gxdT3XLS9OKmCv87vkINQcNEcIIoFWbP9HORz9v3vQwR4e3ksLc2JZOAFK+s
sS5XMEoznzpihEP0PLc4dCBYjbvSD7kxgDwZ+Aj8Q9PkbvE9sIPP7ON0fz095HdThKjiVJe6vofq
+n6b1NBc8XdrQvBmunwxD5nvtTW4vtN6VY7mUCmxsCieuoBJ9OlqmsVWQvifIYf40dJPZkk9YgGT
zWLpXDSfLSplbY2LL9C9U0ptvjcDjefLTvqSFc7tw1sEhF0n/qpA2r0GpvkLRDmcSwVyPvmjFBGq
Up/pNy8ZuPGQmHwFi2/14+xeSUDG2bwnsYJQG2EdJCB6luQ57GEnTA/yKZSTKI8dDQa8Sd3zfXb1
9mOgSF0bBdXbuKhEpuP9wirslFe6fQ1t5j5R0xi72MZ8ikMu1RQZKCyDbMwazlHiMIIHNTCCBh2g
AwIBAgIDA9RBMA0GCSqGSIb3DQEBBQUAMIGMMQswCQYDVQQGEwJJTDEWMBQGA1UEChMNU3RhcnRD
b20gTHRkLjErMCkGA1UECxMiU2VjdXJlIERpZ2l0YWwgQ2VydGlmaWNhdGUgU2lnbmluZzE4MDYG
A1UEAxMvU3RhcnRDb20gQ2xhc3MgMSBQcmltYXJ5IEludGVybWVkaWF0ZSBDbGllbnQgQ0EwHhcN
MTIwMzAzMDExNzE3WhcNMTMwMzAzMTc1MjI1WjBxMRkwFwYDVQQNExBxWnhjTVBMUFY1VzkwRGsy
MSYwJAYDVQQDDB1uaWNob2xhc0BuaWNob2xhc3dpbGxpYW1zLm5ldDEsMCoGCSqGSIb3DQEJARYd
bmljaG9sYXNAbmljaG9sYXN3aWxsaWFtcy5uZXQwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK
AoIBAQDig+zwyZAnK/RdBQ0odOeIMb0hcf3w+M9oMmD++nJ1/Dr5GeyrXPc8yjUu5ZBVnu7Ycsuo
kyc+loxSfRzlDuGI8+QjbxV9NPDJzbhynNA9s5usE3ChksooNT4OlQyOpeJ7vjN87cRAg23uUmca
HmFN4n3B2bgCbMMu1eELM42kWa1AFIBho8Bungg87xE7/u2nfI6/l3Q+evczVN9XIQWSSRd2Mc5P
4K67rs49U/QJT8jEs6lGrcyGnP0D36P4xeOZyQe6txo+yOnCwjYiJqGNOWOof3nNmpL1Kpwz8VAS
WTihY97sZ70N6QN8cywu79kmW+Rjmb7X5VhYtBhHNJ15AgMBAAGjggO4MIIDtDAJBgNVHRMEAjAA
MAsGA1UdDwQEAwIEsDAdBgNVHSUEFjAUBggrBgEFBQcDAgYIKwYBBQUHAwQwHQYDVR0OBBYEFKA2
C1VmyufOxkIzAavxOxhC7l7PMB8GA1UdIwQYMBaAFFNy7ZKc4NrLAVx8fpY1TvLUuFGCMCgGA1Ud
EQQhMB+BHW5pY2hvbGFzQG5pY2hvbGFzd2lsbGlhbXMubmV0MIICIQYDVR0gBIICGDCCAhQwggIQ
BgsrBgEEAYG1NwECAjCCAf8wLgYIKwYBBQUHAgEWImh0dHA6Ly93d3cuc3RhcnRzc2wuY29tL3Bv
bGljeS5wZGYwNAYIKwYBBQUHAgEWKGh0dHA6Ly93d3cuc3RhcnRzc2wuY29tL2ludGVybWVkaWF0
ZS5wZGYwgfcGCCsGAQUFBwICMIHqMCcWIFN0YXJ0Q29tIENlcnRpZmljYXRpb24gQXV0aG9yaXR5
MAMCAQEagb5UaGlzIGNlcnRpZmljYXRlIHdhcyBpc3N1ZWQgYWNjb3JkaW5nIHRvIHRoZSBDbGFz
cyAxIFZhbGlkYXRpb24gcmVxdWlyZW1lbnRzIG9mIHRoZSBTdGFydENvbSBDQSBwb2xpY3ksIHJl
bGlhbmNlIG9ubHkgZm9yIHRoZSBpbnRlbmRlZCBwdXJwb3NlIGluIGNvbXBsaWFuY2Ugb2YgdGhl
IHJlbHlpbmcgcGFydHkgb2JsaWdhdGlvbnMuMIGcBggrBgEFBQcCAjCBjzAnFiBTdGFydENvbSBD
ZXJ0aWZpY2F0aW9uIEF1dGhvcml0eTADAgECGmRMaWFiaWxpdHkgYW5kIHdhcnJhbnRpZXMgYXJl
IGxpbWl0ZWQhIFNlZSBzZWN0aW9uICJMZWdhbCBhbmQgTGltaXRhdGlvbnMiIG9mIHRoZSBTdGFy
dENvbSBDQSBwb2xpY3kuMDYGA1UdHwQvMC0wK6ApoCeGJWh0dHA6Ly9jcmwuc3RhcnRzc2wuY29t
L2NydHUxLWNybC5jcmwwgY4GCCsGAQUFBwEBBIGBMH8wOQYIKwYBBQUHMAGGLWh0dHA6Ly9vY3Nw
LnN0YXJ0c3NsLmNvbS9zdWIvY2xhc3MxL2NsaWVudC9jYTBCBggrBgEFBQcwAoY2aHR0cDovL2Fp
YS5zdGFydHNzbC5jb20vY2VydHMvc3ViLmNsYXNzMS5jbGllbnQuY2EuY3J0MCMGA1UdEgQcMBqG
GGh0dHA6Ly93d3cuc3RhcnRzc2wuY29tLzANBgkqhkiG9w0BAQUFAAOCAQEASConUap58Dj+QUOV
lmPeEZ26BK9NO5+C4SHm/938Zpml/yy8CRLCmJEyFCA+2e11k86FiQpiXcYlo0Rqwvgg3l3dLEu4
sMbsKLzTfKlVXCPHgaJU/rSFZR9qQWDWcYtmoTN1mwMVHXQs/jNPIpAzd4f3MrfIzkW+angTxcak
olNE9CUg7lbXA3gJ6FgGhz3VCsI48g4eISmNg7BHLDH7MNprOBZTFRJ8c9torCqR9zBsVI6xTMQA
oR5Lol7+c3pSc6DNzejtoOA61vJNAjvHzAXZlNQuUwAnkDhfbe/o52vs3iKqBemrUTQGbrFtkZ9Q
9KGcQfhA+MGD+PG4ckSybTGCA28wggNrAgEBMIGUMIGMMQswCQYDVQQGEwJJTDEWMBQGA1UEChMN
U3RhcnRDb20gTHRkLjErMCkGA1UECxMiU2VjdXJlIERpZ2l0YWwgQ2VydGlmaWNhdGUgU2lnbmlu
ZzE4MDYGA1UEAxMvU3RhcnRDb20gQ2xhc3MgMSBQcmltYXJ5IEludGVybWVkaWF0ZSBDbGllbnQg
Q0ECAwPUQTAJBgUrDgMCGgUAoIIBrzAYBgkqhkiG9w0BCQMxCwYJKoZIhvcNAQcBMBwGCSqGSIb3
DQEJBTEPFw0xMzAyMTMxNzA1MzNaMCMGCSqGSIb3DQEJBDEWBBTZbMBQAino7nAs/vWn8gdZ72+h
MTCBpQYJKwYBBAGCNxAEMYGXMIGUMIGMMQswCQYDVQQGEwJJTDEWMBQGA1UEChMNU3RhcnRDb20g
THRkLjErMCkGA1UECxMiU2VjdXJlIERpZ2l0YWwgQ2VydGlmaWNhdGUgU2lnbmluZzE4MDYGA1UE
AxMvU3RhcnRDb20gQ2xhc3MgMSBQcmltYXJ5IEludGVybWVkaWF0ZSBDbGllbnQgQ0ECAwPUQTCB
pwYLKoZIhvcNAQkQAgsxgZeggZQwgYwxCzAJBgNVBAYTAklMMRYwFAYDVQQKEw1TdGFydENvbSBM
dGQuMSswKQYDVQQLEyJTZWN1cmUgRGlnaXRhbCBDZXJ0aWZpY2F0ZSBTaWduaW5nMTgwNgYDVQQD
Ey9TdGFydENvbSBDbGFzcyAxIFByaW1hcnkgSW50ZXJtZWRpYXRlIENsaWVudCBDQQIDA9RBMA0G
CSqGSIb3DQEBAQUABIIBAChn/ZyBWdFR6vJ+9hRzV6Hque8DlT6XkWz4ltfHdQ5GUv/Y0HM1uLdp
Jo/LTESJR08pAwP68g0pKup/MAPmzio44WdyE2bZYmV8dBA6/1wEz9FBWHGgy7Vzrdpk+l3t1Hhz
fCQNP3PM93IWM045H8Hc6x1sAKXiAkLvxf+DZY3cSuv22xCK1y/7ZfEP6QppK7jpxkd53RBmo1y6
Xm5abVTuzlKrg0R5/m1FqGt6W4xANEovX3GUowzYSnXj2xC6P5ByTX3WuyfybXf2+F6lPcZL2CZV
QaovWEtPLOsS9N03IIBGGLM9CJPOovT5/OC7/+xAbQGZhGZLUneqYMuj8+8AAAAAAAA=

--Apple-Mail=_61241DAF-2484-4BAE-B0C5-6E1E4B5830CC--