Re: svn commit: r1351778 - /ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java

[Jacques Le Roux]

I will also have a look. I explained why I did so at https://issues.apache.org/jira/browse/OFBIZ-4289.

Jacques

From: <[hidden email]>

> Author: jacopoc
> Date: Tue Jun 19 17:14:14 2012
> New Revision: 1351778
>
> URL: http://svn.apache.org/viewvc?rev=1351778&view=rev
> Log:
> Setting a default value of DeltaManager was causing a side effect: the method setWebContextObjects was always invoked with the
> persistSerialized argument set to false; this was causing a series of issues (including one reported recently happening when two
> users login/logout to the same application from the same browser and visit another application).
>
> The commit when this bug was introduced is:
>
> Author: jleroux
> Date: Mon Jun  6 20:26:35 2011
> New Revision: 1132749
>
> and unfortunately the revision was backported to the release branches; the commit may have introduced other issues but I can't
> review it now.
>
> Modified:
>    ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>
> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
> URL:
> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java?rev=1351778&r1=1351777&r2=1351778&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java (original)
> +++ ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java Tue Jun 19 17:14:14 2012
> @@ -659,7 +659,7 @@ public class LoginWorker {
>             String mgrClassName = null;
>             try {
>                 cc = ContainerConfig.getContainer("catalina-container", configFile);
> -                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class",
> "org.apache.catalina.ha.session.DeltaManager");
> +                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class", "");
>             } catch (ContainerException e) {
>                 Debug.logError(e, "No catalina-container configuration found in container config!");
>             }
>
>

[Jacques Le Roux-3]

BTW the reported issue was only in a cluster context or also with a sole server?

Jacques

From: "Jacques Le Roux" <[hidden email]>

>I will also have a look. I explained why I did so at https://issues.apache.org/jira/browse/OFBIZ-4289.
>
> Jacques
>
> From: <[hidden email]>
>> Author: jacopoc
>> Date: Tue Jun 19 17:14:14 2012
>> New Revision: 1351778
>>
>> URL: http://svn.apache.org/viewvc?rev=1351778&view=rev
>> Log:
>> Setting a default value of DeltaManager was causing a side effect: the method setWebContextObjects was always invoked with the
>> persistSerialized argument set to false; this was causing a series of issues (including one reported recently happening when two
>> users login/logout to the same application from the same browser and visit another application).
>>
>> The commit when this bug was introduced is:
>>
>> Author: jleroux
>> Date: Mon Jun  6 20:26:35 2011
>> New Revision: 1132749
>>
>> and unfortunately the revision was backported to the release branches; the commit may have introduced other issues but I can't
>> review it now.
>>
>> Modified:
>>    ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>>
>> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java?rev=1351778&r1=1351777&r2=1351778&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java (original)
>> +++ ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java Tue Jun 19 17:14:14 2012
>> @@ -659,7 +659,7 @@ public class LoginWorker {
>>             String mgrClassName = null;
>>             try {
>>                 cc = ContainerConfig.getContainer("catalina-container", configFile);
>> -                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class",
>> "org.apache.catalina.ha.session.DeltaManager");
>> +                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class", "");
>>             } catch (ContainerException e) {
>>                 Debug.logError(e, "No catalina-container configuration found in container config!");
>>             }
>>
>>

[Jacques Le Roux-3]

Ho I see, big mistake actually. I did not thought about the effect of this default value when the manager-class is not set in
catalina-container. I think I should have used org.apache.catalina.ha.session.StandardManager as default rather

There should not be other side effects though.

Jacques

From: "Jacques Le Roux" <[hidden email]>

> BTW the reported issue was only in a cluster context or also with a sole server?
>
> Jacques
>
> From: "Jacques Le Roux" <[hidden email]>
>>I will also have a look. I explained why I did so at https://issues.apache.org/jira/browse/OFBIZ-4289.
>>
>> Jacques
>>
>> From: <[hidden email]>
>>> Author: jacopoc
>>> Date: Tue Jun 19 17:14:14 2012
>>> New Revision: 1351778
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1351778&view=rev
>>> Log:
>>> Setting a default value of DeltaManager was causing a side effect: the method setWebContextObjects was always invoked with the
>>> persistSerialized argument set to false; this was causing a series of issues (including one reported recently happening when two
>>> users login/logout to the same application from the same browser and visit another application).
>>>
>>> The commit when this bug was introduced is:
>>>
>>> Author: jleroux
>>> Date: Mon Jun  6 20:26:35 2011
>>> New Revision: 1132749
>>>
>>> and unfortunately the revision was backported to the release branches; the commit may have introduced other issues but I can't
>>> review it now.
>>>
>>> Modified:
>>>    ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>>>
>>> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java?rev=1351778&r1=1351777&r2=1351778&view=diff
>>> ==============================================================================
>>> --- ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java (original)
>>> +++ ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java Tue Jun 19 17:14:14 2012
>>> @@ -659,7 +659,7 @@ public class LoginWorker {
>>>             String mgrClassName = null;
>>>             try {
>>>                 cc = ContainerConfig.getContainer("catalina-container", configFile);
>>> -                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class",
>>> "org.apache.catalina.ha.session.DeltaManager");
>>> +                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class", "");
>>>             } catch (ContainerException e) {
>>>                 Debug.logError(e, "No catalina-container configuration found in container config!");
>>>             }
>>>
>>>
>

[Jacques Le Roux-3]

From: "Jacques Le Roux" <[hidden email]>
> Ho I see, big mistake actually. I did not thought about the effect of this default value when the manager-class is not set in
> catalina-container. I think I should have used org.apache.catalina.ha.session.StandardManager as default rather

Anyway StandardManager would be the same as empty string as you did, simply not DeltaManager that I check.

Jacques

> There should not be other side effects though.
>
> Jacques
>
> From: "Jacques Le Roux" <[hidden email]>
>> BTW the reported issue was only in a cluster context or also with a sole server?
>>
>> Jacques
>>
>> From: "Jacques Le Roux" <[hidden email]>
>>>I will also have a look. I explained why I did so at https://issues.apache.org/jira/browse/OFBIZ-4289.
>>>
>>> Jacques
>>>
>>> From: <[hidden email]>
>>>> Author: jacopoc
>>>> Date: Tue Jun 19 17:14:14 2012
>>>> New Revision: 1351778
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1351778&view=rev
>>>> Log:
>>>> Setting a default value of DeltaManager was causing a side effect: the method setWebContextObjects was always invoked with the
>>>> persistSerialized argument set to false; this was causing a series of issues (including one reported recently happening when
>>>> two users login/logout to the same application from the same browser and visit another application).
>>>>
>>>> The commit when this bug was introduced is:
>>>>
>>>> Author: jleroux
>>>> Date: Mon Jun  6 20:26:35 2011
>>>> New Revision: 1132749
>>>>
>>>> and unfortunately the revision was backported to the release branches; the commit may have introduced other issues but I can't
>>>> review it now.
>>>>
>>>> Modified:
>>>>    ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>>>>
>>>> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java?rev=1351778&r1=1351777&r2=1351778&view=diff
>>>> ==============================================================================
>>>> --- ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java (original)
>>>> +++ ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java Tue Jun 19 17:14:14 2012
>>>> @@ -659,7 +659,7 @@ public class LoginWorker {
>>>>             String mgrClassName = null;
>>>>             try {
>>>>                 cc = ContainerConfig.getContainer("catalina-container", configFile);
>>>> -                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class",
>>>> "org.apache.catalina.ha.session.DeltaManager");
>>>> +                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class", "");
>>>>             } catch (ContainerException e) {
>>>>                 Debug.logError(e, "No catalina-container configuration found in container config!");
>>>>             }
>>>>
>>>>
>>
>

[Jacques Le Roux-3]

Mmm, we could also remove the whole initial commit and all backports.
Simply all objects put in session would need then to be serialized...

Though it seems I crossed other issues with the dispatcher, security and authz...

Jacques

From: "Jacques Le Roux" <[hidden email]>

> From: "Jacques Le Roux" <[hidden email]>
>> Ho I see, big mistake actually. I did not thought about the effect of this default value when the manager-class is not set in
>> catalina-container. I think I should have used org.apache.catalina.ha.session.StandardManager as default rather
>
> Anyway StandardManager would be the same as empty string as you did, simply not DeltaManager that I check.
>
> Jacques
>
>> There should not be other side effects though.
>>
>> Jacques
>>
>> From: "Jacques Le Roux" <[hidden email]>
>>> BTW the reported issue was only in a cluster context or also with a sole server?
>>>
>>> Jacques
>>>
>>> From: "Jacques Le Roux" <[hidden email]>
>>>>I will also have a look. I explained why I did so at https://issues.apache.org/jira/browse/OFBIZ-4289.
>>>>
>>>> Jacques
>>>>
>>>> From: <[hidden email]>
>>>>> Author: jacopoc
>>>>> Date: Tue Jun 19 17:14:14 2012
>>>>> New Revision: 1351778
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1351778&view=rev
>>>>> Log:
>>>>> Setting a default value of DeltaManager was causing a side effect: the method setWebContextObjects was always invoked with the
>>>>> persistSerialized argument set to false; this was causing a series of issues (including one reported recently happening when
>>>>> two users login/logout to the same application from the same browser and visit another application).
>>>>>
>>>>> The commit when this bug was introduced is:
>>>>>
>>>>> Author: jleroux
>>>>> Date: Mon Jun  6 20:26:35 2011
>>>>> New Revision: 1132749
>>>>>
>>>>> and unfortunately the revision was backported to the release branches; the commit may have introduced other issues but I can't
>>>>> review it now.
>>>>>
>>>>> Modified:
>>>>>    ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>>>>>
>>>>> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java?rev=1351778&r1=1351777&r2=1351778&view=diff
>>>>> ==============================================================================
>>>>> --- ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java (original)
>>>>> +++ ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java Tue Jun 19 17:14:14 2012
>>>>> @@ -659,7 +659,7 @@ public class LoginWorker {
>>>>>             String mgrClassName = null;
>>>>>             try {
>>>>>                 cc = ContainerConfig.getContainer("catalina-container", configFile);
>>>>> -                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class",
>>>>> "org.apache.catalina.ha.session.DeltaManager");
>>>>> +                mgrClassName = ContainerConfig.getPropertyValue(cc, "manager-class", "");
>>>>>             } catch (ContainerException e) {
>>>>>                 Debug.logError(e, "No catalina-container configuration found in container config!");
>>>>>             }
>>>>>
>>>>>
>>>
>>
>

[Jacopo Cappellato-4]

On Jun 19, 2012, at 10:46 PM, Jacques Le Roux wrote:

> Mmm, we could also remove the whole initial commit and all backports.

This would be ideal... the whole commit seems to add unwanted complexity just to handle a specific use case that could probably be managed with different approaches,if properly discussed/designed with the community.

> Simply all objects put in session would need then to be serialized...

This was the first thing I thought when I discovered the bug and your commit while I was debugging the error yesterday.

Kind regards,

Jacopo

[Jacques Le Roux]

I slept on it and here is what I will do.

When using DeltaManager, I remember crossing issues with session serialisation at logout (due to Java Objects in session and, as I
said, also dispatcher, security and authz object, though I don't clearly remember for the last 3).
Now that I think about it, what I wrote there was an ugly workaround that I should never have committed as is.

What I propose:
revert your change Jacopo
revert my initial commit and related backports

Because I want to be able sometimes to use DeltaManager with only its distributable capabilities not session persistence (which
comes with some troubles)
I want to introduce later the same workaround but not as I did. I'd want to use a sessionPersistence bollean properties rather, to
make things totally independent from the Manager
Anyway it will be only when I will get enough time to properly investigate this again...

Jacques

From: "Jacopo Cappellato" <[hidden email]>

> On Jun 19, 2012, at 10:46 PM, Jacques Le Roux wrote:
>
>> Mmm, we could also remove the whole initial commit and all backports.
>
> This would be ideal... the whole commit seems to add unwanted complexity just to handle a specific use case that could probably be
> managed with different approaches,if properly discussed/designed with the community.
>
>> Simply all objects put in session would need then to be serialized...
>
> This was the first thing I thought when I discovered the bug and your commit while I was debugging the error yesterday.
>
> Kind regards,
>
> Jacopo
>
>

[Jacopo Cappellato-4]

On Jun 20, 2012, at 10:12 AM, Jacques Le Roux wrote:

> I slept on it and here is what I will do.
>
> When using DeltaManager, I remember crossing issues with session serialisation at logout (due to Java Objects in session and, as I said, also dispatcher, security and authz object, though I don't clearly remember for the last 3).
> Now that I think about it, what I wrote there was an ugly workaround that I should never have committed as is.
>
> What I propose:
> revert your change Jacopo
> revert my initial commit and related backports
>
> Because I want to be able sometimes to use DeltaManager with only its distributable capabilities not session persistence (which comes with some troubles)
> I want to introduce later the same workaround but not as I did. I'd want to use a sessionPersistence bollean properties rather, to make things totally independent from the Manager
> Anyway it will be only when I will get enough time to properly investigate this again...

Ok, for a change like this I would like to review your solution before it is committed, so when you will implement the changes that you would like to contribute, please share the patch for community review.

Thanks,

Jacopo

>
> Jacques
>
> From: "Jacopo Cappellato" <[hidden email]>
>> On Jun 19, 2012, at 10:46 PM, Jacques Le Roux wrote:
>>
>>> Mmm, we could also remove the whole initial commit and all backports.
>>
>> This would be ideal... the whole commit seems to add unwanted complexity just to handle a specific use case that could probably be managed with different approaches,if properly discussed/designed with the community.
>>
>>> Simply all objects put in session would need then to be serialized...
>>
>> This was the first thing I thought when I discovered the bug and your commit while I was debugging the error yesterday.
>>
>> Kind regards,
>>
>> Jacopo
>>

[Jacques Le Roux]

From: "Jacopo Cappellato" <[hidden email]>

> On Jun 20, 2012, at 10:12 AM, Jacques Le Roux wrote:
>
>> I slept on it and here is what I will do.
>>
>> When using DeltaManager, I remember crossing issues with session serialisation at logout (due to Java Objects in session and, as
>> I said, also dispatcher, security and authz object, though I don't clearly remember for the last 3).
>> Now that I think about it, what I wrote there was an ugly workaround that I should never have committed as is.
>>
>> What I propose:
>> revert your change Jacopo
>> revert my initial commit and related backports
>>
>> Because I want to be able sometimes to use DeltaManager with only its distributable capabilities not session persistence (which
>> comes with some troubles)
>> I want to introduce later the same workaround but not as I did. I'd want to use a sessionPersistence bollean properties rather,
>> to make things totally independent from the Manager
>> Anyway it will be only when I will get enough time to properly investigate this again...
>
> Ok, for a change like this I would like to review your solution before it is committed, so when you will implement the changes
> that you would like to contribute, please share the patch for community review.

Sure, actually it will be the same but will rely on a new session-persistence boolean property (true by default, hence not
functional changes OOTB) instead of the manager-class value which made any sense.

I will propose a patch in a Jira soon...

Jacques

> Thanks,
>
> Jacopo
>
>>
>> Jacques
>>
>> From: "Jacopo Cappellato" <[hidden email]>
>>> On Jun 19, 2012, at 10:46 PM, Jacques Le Roux wrote:
>>>
>>>> Mmm, we could also remove the whole initial commit and all backports.
>>>
>>> This would be ideal... the whole commit seems to add unwanted complexity just to handle a specific use case that could probably
>>> be managed with different approaches,if properly discussed/designed with the community.
>>>
>>>> Simply all objects put in session would need then to be serialized...
>>>
>>> This was the first thing I thought when I discovered the bug and your commit while I was debugging the error yesterday.
>>>
>>> Kind regards,
>>>
>>> Jacopo
>>>
>
>

[Jacques Le Roux]

From: "Jacques Le Roux" <[hidden email]>

> From: "Jacopo Cappellato" <[hidden email]>
>> On Jun 20, 2012, at 10:12 AM, Jacques Le Roux wrote:
>>
>>> I slept on it and here is what I will do.
>>>
>>> When using DeltaManager, I remember crossing issues with session serialisation at logout (due to Java Objects in session and, as
>>> I said, also dispatcher, security and authz object, though I don't clearly remember for the last 3).
>>> Now that I think about it, what I wrote there was an ugly workaround that I should never have committed as is.
>>>
>>> What I propose:
>>> revert your change Jacopo
>>> revert my initial commit and related backports
>>>
>>> Because I want to be able sometimes to use DeltaManager with only its distributable capabilities not session persistence (which
>>> comes with some troubles)
>>> I want to introduce later the same workaround but not as I did. I'd want to use a sessionPersistence bollean properties rather,
>>> to make things totally independent from the Manager
>>> Anyway it will be only when I will get enough time to properly investigate this again...
>>
>> Ok, for a change like this I would like to review your solution before it is committed, so when you will implement the changes
>> that you would like to contribute, please share the patch for community review.
>
> Sure, actually it will be the same but will rely on a new session-persistence boolean property (true by default, hence not
> functional changes OOTB) instead of the manager-class value which made any sense.
>
> I will propose a patch in a Jira soon...

Please see  https://issues.apache.org/jira/browse/OFBIZ-4289

I will have a look at the JMS/DCC issue later in the weekend...

Jacques

> Jacques
>
>> Thanks,
>>
>> Jacopo
>>
>>>
>>> Jacques
>>>
>>> From: "Jacopo Cappellato" <[hidden email]>
>>>> On Jun 19, 2012, at 10:46 PM, Jacques Le Roux wrote:
>>>>
>>>>> Mmm, we could also remove the whole initial commit and all backports.
>>>>
>>>> This would be ideal... the whole commit seems to add unwanted complexity just to handle a specific use case that could probably
>>>> be managed with different approaches,if properly discussed/designed with the community.
>>>>
>>>>> Simply all objects put in session would need then to be serialized...
>>>>
>>>> This was the first thing I thought when I discovered the bug and your commit while I was debugging the error yesterday.
>>>>
>>>> Kind regards,
>>>>
>>>> Jacopo
>>>>
>>
>>