OFBiz
  1. OFBiz
  2. OFBIZ-4289

Login out on a cluster handled by DeltaManager causes a NPE

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: Release Branch 4.0, Release Branch 09.04, Release Branch 10.04, Release Branch 11.04, Trunk
    • Fix Version/s: Release Branch 12.04, Trunk
    • Component/s: framework
    • Labels:
    • Environment:

      Ubuntu

      Description

      It works locally but not on clusters. You simply get a NPE.

      Problem

      • When we logout we cross an issue due to Tomcat sessions persistence.
      • Because we set distributable to true, to allow sessions failover,
      • and use and DeltaManager for replication.
      • Delegator and other main Classes (notably Dispatcher) are not serialized in OFBiz. This is the origin of the problem

      Tried solutions

      By default DeltaManager save active sessions on disk. It uses a private String variable pathname for that (default to "SESSIONS.ser"). You can set it to null, to avoid session persistence, using a context.xml file in the WEB-INF folder with content like below. But I tried it in webtools app on staging qs001 (only) and it did not work (with distributable set to false). And we have no other means to set it from OFBiz (OOTB).

          <Context>
           <Manager className="org.apache.catalina.session.DeltatManager" pathname="">
           </Manager>
          </Context>
      

        Activity

        Hide
        Jacques Le Roux added a comment - - edited

        I first wrote a changeset_2510.diff patch which works locally and on cluster but does not take into account the Manager type. I think it's ok but to be sure I have also created a patch with no possible regression: OFBIZ-4289_Login_out_on_a cluster_handled_by_DeltaManager_causes_a_NPE.patch

        So changeset_2510.diff is not intended for inclusion. But I'm curious to know why it works. Tough I have no time to digg in further on why we need to put delegator, dispatcher, security and authorization in session. So if someone is interested...

        Show
        Jacques Le Roux added a comment - - edited I first wrote a changeset_2510.diff patch which works locally and on cluster but does not take into account the Manager type. I think it's ok but to be sure I have also created a patch with no possible regression: OFBIZ-4289 _Login_out_on_a cluster_handled_by_DeltaManager_causes_a_NPE.patch So changeset_2510.diff is not intended for inclusion. But I'm curious to know why it works. Tough I have no time to digg in further on why we need to put delegator, dispatcher, security and authorization in session. So if someone is interested...
        Hide
        Jacques Le Roux added a comment -

        I must say I did not test yet the OFBIZ-4289_Login_out_on_a cluster_handled_by_DeltaManager_causes_a_NPE.patch on a cluster, will do Monday (I'm confident)

        Show
        Jacques Le Roux added a comment - I must say I did not test yet the OFBIZ-4289 _Login_out_on_a cluster_handled_by_DeltaManager_causes_a_NPE.patch on a cluster, will do Monday (I'm confident)
        Hide
        Jacques Le Roux added a comment -

        Apart a typo, it was OK on cluster also. So I will commit in a week if nobody see a problem until then

        Show
        Jacques Le Roux added a comment - Apart a typo, it was OK on cluster also. So I will commit in a week if nobody see a problem until then
        Hide
        Jacques Le Roux added a comment -

        Commited at

        trunk: r1132749
        R11.04: r1132758
        R10.04: r1132754

        Show
        Jacques Le Roux added a comment - Commited at trunk: r1132749 R11.04: r1132758 R10.04: r1132754
        Hide
        Jacques Le Roux added a comment -

        This was not well done, Jacopo had to put a "" default Manager

        I have a better idea...

        Show
        Jacques Le Roux added a comment - This was not well done, Jacopo had to put a "" default Manager I have a better idea...
        Hide
        Jacques Le Roux added a comment -

        Correct solution taking into account other possible container files. What I wrote before was plainly wrong.

        Show
        Jacques Le Roux added a comment - Correct solution taking into account other possible container files. What I wrote before was plainly wrong.
        Hide
        Jacopo Cappellato added a comment -

        Jacques,

        here are some comments:
        a) I did a quick review but it seems that staticContainerConfig is not thread safe
        b) there are screens in OFBiz that cause issues if the security objects are not in the context (when you login/logout and login with a different user in the same browser and visit another application): these errors will happen again if sessionPersistence is enabled (e.g. in clusters)

        Jacopo

        PS: I think there are some unwanted changes in your patch:
        1)

        -                <property name="mcast-bind-addr" value="192.168.2.1"/>
        +                <property name="mcast-bind-addr" value="192.168.1.8"/>
                         <property name="mcast-addr" value="228.0.0.4"/>
                         <property name="mcast-port" value="45564"/>
                         <property name="mcast-freq" value="500"/>
                         <property name="mcast-drop-time" value="3000"/>
                     </property>
        -            -->
        

        2) framework/service/src/org/ofbiz/service/job/JobManager.java.patch

        Show
        Jacopo Cappellato added a comment - Jacques, here are some comments: a) I did a quick review but it seems that staticContainerConfig is not thread safe b) there are screens in OFBiz that cause issues if the security objects are not in the context (when you login/logout and login with a different user in the same browser and visit another application): these errors will happen again if sessionPersistence is enabled (e.g. in clusters) Jacopo PS: I think there are some unwanted changes in your patch: 1) - <property name= "mcast-bind-addr" value= "192.168.2.1" /> + <property name= "mcast-bind-addr" value= "192.168.1.8" /> <property name= "mcast-addr" value= "228.0.0.4" /> <property name= "mcast-port" value= "45564" /> <property name= "mcast-freq" value= "500" /> <property name= "mcast-drop-time" value= "3000" /> </property> - --> 2) framework/service/src/org/ofbiz/service/job/JobManager.java.patch
        Hide
        Jacques Le Roux added a comment -

        Thanks for review,

        I have updated the patch removing the unwanted changes you reported.

        a) What would be the problems with staticContainerConfig? It's a simple String, there can be only one instance (either default ofbiz-container or passed by java cmd line )and it's only read.
        b) Could you please give me a such screen? BTW, IIRW, I just spotted the NPEs in log and they had no "real" side-effects. So maybe we could get rid of all that. It begins to get a bit complicated for no "real" problems. I have to check though...

        Show
        Jacques Le Roux added a comment - Thanks for review, I have updated the patch removing the unwanted changes you reported. a) What would be the problems with staticContainerConfig? It's a simple String, there can be only one instance (either default ofbiz-container or passed by java cmd line )and it's only read. b) Could you please give me a such screen? BTW, IIRW, I just spotted the NPEs in log and they had no "real" side-effects. So maybe we could get rid of all that. It begins to get a bit complicated for no "real" problems. I have to check though...
        Hide
        Jacopo Cappellato added a comment -

        As regards #b, the issue could be recreated (before my fix) in the following way:
        1) log in to catalog as admin/ofbiz
        2) log out
        3) log in to catalog as flexadmin/ofbiz
        4) visit the order application --> the screen blows up because "security" is null

        Show
        Jacopo Cappellato added a comment - As regards #b, the issue could be recreated (before my fix) in the following way: 1) log in to catalog as admin/ofbiz 2) log out 3) log in to catalog as flexadmin/ofbiz 4) visit the order application --> the screen blows up because "security" is null
        Hide
        Jacopo Cappellato added a comment -

        As regards #a: staticContainerConfig could be set by a thread while another thread is reading it; unfortunately this is true with several other fields in that class; however a static field is even worse because the risk of accessing the variable when not initialized is greater.

        Show
        Jacopo Cappellato added a comment - As regards #a: staticContainerConfig could be set by a thread while another thread is reading it; unfortunately this is true with several other fields in that class; however a static field is even worse because the risk of accessing the variable when not initialized is greater.
        Hide
        Jacopo Cappellato added a comment -

        Additionally, staticContainerConfig should not be public.

        Show
        Jacopo Cappellato added a comment - Additionally, staticContainerConfig should not be public.
        Hide
        Jacques Le Roux added a comment -

        It was easier to revert all changes related to this issue than continuing because, IIRW, this was not a blocking NPE. It was just throwed in log when non serialised Objects where put in session.

        Reverted at
        trunk r1353135
        R11.04 r1353136
        R10.04 r1353137

        Show
        Jacques Le Roux added a comment - It was easier to revert all changes related to this issue than continuing because, IIRW, this was not a blocking NPE. It was just throwed in log when non serialised Objects where put in session. Reverted at trunk r1353135 R11.04 r1353136 R10.04 r1353137
        Hide
        Jacques Le Roux added a comment -

        Will rather close as to be fixed later

        Show
        Jacques Le Roux added a comment - Will rather close as to be fixed later
        Hide
        Jacques Le Roux added a comment - - edited

        == TYPO ==
        OK, the NPE is really a blocking problem when you set <distributable/> in concerned applications

        Show
        Jacques Le Roux added a comment - - edited == TYPO == OK, the NPE is really a blocking problem when you set <distributable/> in concerned applications
        Hide
        Jacques Le Roux added a comment -

        Also if you try to put aside security you get:
        org.ofbiz.webapp.event.EventHandlerException: Problems processing event: java.lang.IllegalArgumentException: setAttribute: Non-serializable attribute security (setAttribute: Non-serializable attribute security)

        Show
        Jacques Le Roux added a comment - Also if you try to put aside security you get: org.ofbiz.webapp.event.EventHandlerException: Problems processing event: java.lang.IllegalArgumentException: setAttribute: Non-serializable attribute security (setAttribute: Non-serializable attribute security)
        Hide
        Jacques Le Roux added a comment -

        So I think we should better reload security when it's missing than hidding these real issues

        Show
        Jacques Le Roux added a comment - So I think we should better reload security when it's missing than hidding these real issues
        Hide
        Jacques Le Roux added a comment -

        I believe this patch is finally the right way to do it. At least I don't see a better/simpler way...

        There were no needs to reload security when missing. I simply did it too fast, and was not keeping security and authz in the request.

        It's much more simple now and only dependent on apps-distributable in catalina-container.

        Please review and test

        Show
        Jacques Le Roux added a comment - I believe this patch is finally the right way to do it. At least I don't see a better/simpler way... There were no needs to reload security when missing. I simply did it too fast, and was not keeping security and authz in the request. It's much more simple now and only dependent on apps-distributable in catalina-container. Please review and test
        Hide
        Jacques Le Roux added a comment -

        Ha wait, I will remove the dependency on Config.java, like I did in earlier patches...

        Show
        Jacques Le Roux added a comment - Ha wait, I will remove the dependency on Config.java, like I did in earlier patches...
        Hide
        Jacques Le Roux added a comment -

        Mmm, then you have to hardcode the containers file name :/. I keep it as is...

        Show
        Jacques Le Roux added a comment - Mmm, then you have to hardcode the containers file name :/. I keep it as is...
        Hide
        Jacopo Cappellato added a comment -

        A few comments:

        • the name of the argument "persistSerialized" (in setWebContextObjects method) is confusing and could be renamed to "distributableApplication", it will make the code more readable
        • we should also rename the variable "sessionPersistence" (same as above)
        • in setWebContextObjects instead of:
                  if (!persistSerialized) {
                      session.setAttribute("delegator", null);
                  } else {
                      session.setAttribute("delegator", delegator);
                  }
          

          it will be more readable as:

                  if (persistSerialized) {
                      session.setAttribute("delegator", delegator);
                  } else {
                      session.setAttribute("delegator", null);
                  }
          

          or, if you will implement my suggestion #1:

                  if (distributableApplication) {
                      session.setAttribute("delegator", null);
                  } else {
                      session.setAttribute("delegator", delegator);
                  }
          
        • in order to increase readability we could group all the special code for distributable application in the same block:
                  if (distributableApplication) {
                      session.setAttribute("delegator", null);
                      session.setAttribute("dispatcher", null);
                      session.setAttribute("security", null);
                      session.setAttribute("authz", null);
                  } else {
                      session.setAttribute("delegator", delegator);
                      session.setAttribute("dispatcher", dispatcher);
                      session.setAttribute("security", security);
                      session.setAttribute("authz", authz);
                  }
          

          ; this is actually minor; however see my next 2 comments

        • did you consider to not change the code here but instead, before the session is serialized, remove all the objects that are not "instanceof Serializable"?
        • what is the effect (in distributable appls) of setting nulls for security/dispatcher/delegator etc...? isn't it causing any issue? aren't they used?
        Show
        Jacopo Cappellato added a comment - A few comments: the name of the argument "persistSerialized" (in setWebContextObjects method) is confusing and could be renamed to "distributableApplication", it will make the code more readable we should also rename the variable "sessionPersistence" (same as above) in setWebContextObjects instead of: if (!persistSerialized) { session.setAttribute( "delegator" , null ); } else { session.setAttribute( "delegator" , delegator); } it will be more readable as: if (persistSerialized) { session.setAttribute( "delegator" , delegator); } else { session.setAttribute( "delegator" , null ); } or, if you will implement my suggestion #1: if (distributableApplication) { session.setAttribute( "delegator" , null ); } else { session.setAttribute( "delegator" , delegator); } in order to increase readability we could group all the special code for distributable application in the same block: if (distributableApplication) { session.setAttribute( "delegator" , null ); session.setAttribute( "dispatcher" , null ); session.setAttribute( "security" , null ); session.setAttribute( "authz" , null ); } else { session.setAttribute( "delegator" , delegator); session.setAttribute( "dispatcher" , dispatcher); session.setAttribute( "security" , security); session.setAttribute( "authz" , authz); } ; this is actually minor; however see my next 2 comments did you consider to not change the code here but instead, before the session is serialized, remove all the objects that are not "instanceof Serializable"? what is the effect (in distributable appls) of setting nulls for security/dispatcher/delegator etc...? isn't it causing any issue? aren't they used?
        Hide
        Jacques Le Roux added a comment -

        Also Config.getContainerConfig() really does not need to be synchronized, there are no worries:

        1. staticContainerConfig is set once when Config.getInstance() in Start.init() and will then never change
        2. Config.getContainerConfig() can't be called from LoginWorker.doBasicLogout() before OFBIz is up, so it should never be empty, and even in this case it will be handled (persistence will be used). Of course it should not be called before OFBIz is up, does not make sense to do so anyway.
        3. staticContainerConfig is only an always read

        So if we agree on committint this, I will remove synchronized from Config.getContainerConfig(), it's useless

        Show
        Jacques Le Roux added a comment - Also Config.getContainerConfig() really does not need to be synchronized, there are no worries: staticContainerConfig is set once when Config.getInstance() in Start.init() and will then never change Config.getContainerConfig() can't be called from LoginWorker.doBasicLogout() before OFBIz is up, so it should never be empty, and even in this case it will be handled (persistence will be used). Of course it should not be called before OFBIz is up, does not make sense to do so anyway. staticContainerConfig is only an always read So if we agree on committint this, I will remove synchronized from Config.getContainerConfig(), it's useless
        Hide
        Jacques Le Roux added a comment - - edited

        == TYPO ==
        Ha we crossed on wire...

        1. I agree on the name
        2. instanceof Serializable: I did not consider it. Maybe it's better to let developper catch them and decide about it. In OFBiz we are not used to serialise all classes...
        3. I did not find any issues doing this. But I must say the application which was using this in production (during 1 month, before, for other reasons, I decided to turn to round-robin LB and sticky sessions) is really a pure eCommerce application. Only Webtools and totally new and specific backend applications are used else.
        Show
        Jacques Le Roux added a comment - - edited == TYPO == Ha we crossed on wire... I agree on the name instanceof Serializable: I did not consider it. Maybe it's better to let developper catch them and decide about it. In OFBiz we are not used to serialise all classes... I did not find any issues doing this. But I must say the application which was using this in production (during 1 month, before, for other reasons, I decided to turn to round-robin LB and sticky sessions) is really a pure eCommerce application. Only Webtools and totally new and specific backend applications are used else.
        Hide
        Jacopo Cappellato added a comment -

        Jacques,

        I didn't note the synchronized statement; yes please remove it because it is the wrong way to make it thread safe; however I disagree that the variable is immutable; it would be immutable if set in a static initializer and marked as final, which is not the case here.
        However it is not the only example non thread safe fields in that class and the way the class is currently used is fine, so this is a minor issue; however it is not really a good pattern to define a static variable that should only be used after an object of that class is created (it is like of an implicit singleton).

        Show
        Jacopo Cappellato added a comment - Jacques, I didn't note the synchronized statement; yes please remove it because it is the wrong way to make it thread safe; however I disagree that the variable is immutable; it would be immutable if set in a static initializer and marked as final, which is not the case here. However it is not the only example non thread safe fields in that class and the way the class is currently used is fine, so this is a minor issue; however it is not really a good pattern to define a static variable that should only be used after an object of that class is created (it is like of an implicit singleton).
        Hide
        Jacopo Cappellato added a comment -

        The synchronized accessor can be removed because it doesn't make sense to synchronized the getter if you do not synchronize also the code that change the value of the variable.

        Show
        Jacopo Cappellato added a comment - The synchronized accessor can be removed because it doesn't make sense to synchronized the getter if you do not synchronize also the code that change the value of the variable.
        Hide
        Jacques Le Roux added a comment -

        I suppose the way this class is used should not change. But yes I agree we could do better there...

        Show
        Jacques Le Roux added a comment - I suppose the way this class is used should not change. But yes I agree we could do better there...
        Hide
        Jacopo Cappellato added a comment -

        Jacques about your point:

        3. I did not find any issues doing this. But I must say the application which was using this in production (during 1 month, before, for other reasons, I decided to turn to round-robin LB and sticky sessions) is really a pure eCommerce application. Only Webtools and totally new and specific backend applications are used else.

        I suspect that you simply didn't test the proper code, unless these session objects are completely unused (I didn't check but I suspect they are actually used). We should better check how they are used and when before setting them to null.
        It is like repairing a broken engine by pulling out broken parts without replacing them... if everything works fine then there was no reason for keeping them in even if they were not broken.

        Show
        Jacopo Cappellato added a comment - Jacques about your point: 3. I did not find any issues doing this. But I must say the application which was using this in production (during 1 month, before, for other reasons, I decided to turn to round-robin LB and sticky sessions) is really a pure eCommerce application. Only Webtools and totally new and specific backend applications are used else. I suspect that you simply didn't test the proper code, unless these session objects are completely unused (I didn't check but I suspect they are actually used). We should better check how they are used and when before setting them to null. It is like repairing a broken engine by pulling out broken parts without replacing them... if everything works fine then there was no reason for keeping them in even if they were not broken.
        Hide
        Jacopo Cappellato added a comment -

        Jacques, about your comment:

        instanceof Serializable: I did not consider it. Maybe it's better to let developper catch them and decide about it. In OFBiz we are not used to serialise all classes...

        I was not suggesting to add Serializable to OFBiz classes; I was saying that, if the issue you are facing in distributable app mode is that the session contains non serializable objects (e.g. "security" etc..) then, instead of hardcoding the code in LoginWorker to null them, it would be more elegant (impossible) to hook before the session is serialized and iterate thru the objects in the session and null the ones that don't implement Serializable. In this way, if the system is running a serializable Security implementation, then it will be stored in the session even if in distributable mode.
        The code would be also cleaner (no need to add variables and special handling in LoginWorker)

        Show
        Jacopo Cappellato added a comment - Jacques, about your comment: instanceof Serializable: I did not consider it. Maybe it's better to let developper catch them and decide about it. In OFBiz we are not used to serialise all classes... I was not suggesting to add Serializable to OFBiz classes; I was saying that, if the issue you are facing in distributable app mode is that the session contains non serializable objects (e.g. "security" etc..) then, instead of hardcoding the code in LoginWorker to null them, it would be more elegant (impossible) to hook before the session is serialized and iterate thru the objects in the session and null the ones that don't implement Serializable. In this way, if the system is running a serializable Security implementation, then it will be stored in the session even if in distributable mode. The code would be also cleaner (no need to add variables and special handling in LoginWorker)
        Hide
        Jacques Le Roux added a comment -

        I did not get what you mean by

        if everything works fine then there was no reason for keeping them in even if they were not broken.

        What I know is that if you try to serialize them you get a NPE (from Tomcat) because they can't be serialised. In other words it works in a non distributed context but, as soon as you want to distribute, login out fails, because you need to persist sessions.

        On the other hands, I agree on this

        We should better check how they are used and when before setting them to null.

        But what will you do then: make them serializable? Or maybe rather change the code which is supposed to use them...

        was not suggesting to add Serializable to OFBiz classes;

        I did not understand that. I was simply suggesting that developers may add other Objects in session (than the ones we are concerned so far) that would be non serialised, ignoring the issue with logout in distributed applications. But they may want and be able to serialise them. With what you are suggesting this will be hidden and will not let them cope with the issue they may ignore (this is not an obvious issue before you stumble upon it, believe me). For instance (I hope) nobody was aware of this issue before I found it.

        I was saying that, if the issue you are facing in distributable app mode is that the session contains non serializable objects (e.g. "security" etc..) then, instead of hardcoding the code in LoginWorker to null them, it would be more elegant (impossible) to hook before the session is serialized and iterate thru the objects in the session and null the ones that don't implement Serializable. In this way, if the system is running a serializable Security implementation, then it will be stored in the session even if in distributable mode.
        The code would be also cleaner (no need to add variables and special handling in LoginWorker)

        This is going much further than expected. First we must know why those are put in session, what is the need?

        hook before the session is serialized

        This is done by Tomcat (actually the implementation of ManagerBase used) and, if possible, I guess would need much more work than clarifying the fact that those are really needed in session when login out...

        Show
        Jacques Le Roux added a comment - I did not get what you mean by if everything works fine then there was no reason for keeping them in even if they were not broken. What I know is that if you try to serialize them you get a NPE (from Tomcat) because they can't be serialised. In other words it works in a non distributed context but, as soon as you want to distribute, login out fails, because you need to persist sessions. On the other hands, I agree on this We should better check how they are used and when before setting them to null. But what will you do then: make them serializable? Or maybe rather change the code which is supposed to use them... was not suggesting to add Serializable to OFBiz classes; I did not understand that. I was simply suggesting that developers may add other Objects in session (than the ones we are concerned so far) that would be non serialised, ignoring the issue with logout in distributed applications. But they may want and be able to serialise them. With what you are suggesting this will be hidden and will not let them cope with the issue they may ignore (this is not an obvious issue before you stumble upon it, believe me). For instance (I hope) nobody was aware of this issue before I found it. I was saying that, if the issue you are facing in distributable app mode is that the session contains non serializable objects (e.g. "security" etc..) then, instead of hardcoding the code in LoginWorker to null them, it would be more elegant (impossible) to hook before the session is serialized and iterate thru the objects in the session and null the ones that don't implement Serializable. In this way, if the system is running a serializable Security implementation, then it will be stored in the session even if in distributable mode. The code would be also cleaner (no need to add variables and special handling in LoginWorker) This is going much further than expected. First we must know why those are put in session, what is the need? hook before the session is serialized This is done by Tomcat (actually the implementation of ManagerBase used) and, if possible, I guess would need much more work than clarifying the fact that those are really needed in session when login out...
        Hide
        Jacques Le Roux added a comment - - edited

        == ADDING SOME WORDS ==
        To keep is short, I think there is no reasons to dicuss this any longer as long as we don't clarify the need/s on those 4 Objects in session at logout. Also of course developers who want to distribute their application should either remove non serialised Objects from session or serialise them. And this is not obvious when you develop and application and after (have to) decide that it should be distributed (and was not in initial requirements).

        Show
        Jacques Le Roux added a comment - - edited == ADDING SOME WORDS == To keep is short, I think there is no reasons to dicuss this any longer as long as we don't clarify the need/s on those 4 Objects in session at logout. Also of course developers who want to distribute their application should either remove non serialised Objects from session or serialise them. And this is not obvious when you develop and application and after (have to) decide that it should be distributed (and was not in initial requirements).
        Hide
        Jacopo Cappellato added a comment -

        Jacques, in short I don't think it is a proper fix to set them to null just to avoid the error during serialization, unless you also provide a mechanism to reinsert them when the session is deserialized OR refactor the code to not require non serializable objects in the session OR refactor code to make them serializable.
        I mean, we have to fix the root cause of the issue and not just the effects.

        Show
        Jacopo Cappellato added a comment - Jacques, in short I don't think it is a proper fix to set them to null just to avoid the error during serialization, unless you also provide a mechanism to reinsert them when the session is deserialized OR refactor the code to not require non serializable objects in the session OR refactor code to make them serializable. I mean, we have to fix the root cause of the issue and not just the effects.
        Hide
        Jacopo Cappellato added a comment -

        Just read your last comment and yes I agree with what you wrote: before implementing a fix we should research and understand why OFBiz requires these non serializable objects in the session; one we realize this we can design a proper fix, like:

        • refactor code so that they are not needed and can be removed
        • make them serializable (if possible)
        • write specific code to remove them before serialization and read them after deserialization
        Show
        Jacopo Cappellato added a comment - Just read your last comment and yes I agree with what you wrote: before implementing a fix we should research and understand why OFBiz requires these non serializable objects in the session; one we realize this we can design a proper fix, like: refactor code so that they are not needed and can be removed make them serializable (if possible) write specific code to remove them before serialization and read them after deserialization
        Hide
        Jacques Le Roux added a comment -

        New patch w/out sync for getContainerConfig(), suggested var name by Jacopo and small refactoring but not as suggested because of the catch blocks

        Also I noticed that just before all this we do

        session.setAttribute("delegatorName", delegatorName);

        So in session we have all the needed to regenerate the 4 vars variables based on an initial

        delegator = DelegatorFactory.getDelegator(session.getAttribute("delegatorName"));

        If ever those are needed. So it's not a bid deal...

        Show
        Jacques Le Roux added a comment - New patch w/out sync for getContainerConfig(), suggested var name by Jacopo and small refactoring but not as suggested because of the catch blocks Also I noticed that just before all this we do session.setAttribute("delegatorName", delegatorName); So in session we have all the needed to regenerate the 4 vars variables based on an initial delegator = DelegatorFactory.getDelegator(session.getAttribute("delegatorName")); If ever those are needed. So it's not a bid deal...
        Hide
        Jacques Le Roux added a comment -

        Again crossed on wire, what do you think about my last point. Seems that we have a mean to

        • write specific code to remove them before serialization and read them after deserialization
        Show
        Jacques Le Roux added a comment - Again crossed on wire, what do you think about my last point. Seems that we have a mean to write specific code to remove them before serialization and read them after deserialization
        Hide
        Jacopo Cappellato added a comment - - edited

        Jacques, am I wrong orin this last patch the meaning of the distributableApplication field is completely inverted?

        I also still see the pattern:

        if (!variable) {//false} else {//true}

        rather than the easier to read:

        if (variable) {//true} else {//false}

        Also, but this is a minor note, when I suggested to group together all the code related to distributable apple I was thinking to refactor the code in this way:

                    Security security = null;
                    try {
                        security = SecurityFactory.getInstance(delegator);
                    } catch (SecurityConfigurationException e) {
                        Debug.logError(e, module);
                    }
                    Authorization authz = null;
                    try {
                        authz = AuthorizationFactory.getInstance(delegator);
                    } catch (SecurityConfigurationException e) {
                        Debug.logError(e, module);
                    }
                    // set all request attributes here:
                    request.setAttribute("dispatcher", dispatcher);
                    request.setAttribute("delegator", delegator);
                    request.setAttribute("security", security);
                    request.setAttribute("authz", authz);
                    // set all session attributes here:
                    if (distributableApplication) {
                        // these attributes are nulled here because they are not serializable and then recreated when the session is deserialized
                        session.setAttribute("dispatcher", null);
                        session.setAttribute("delegator", null);
                        session.setAttribute("security", null);
                        session.setAttribute("authz", null);
                    } else {
                        session.setAttribute("dispatcher", dispatcher);
                        session.setAttribute("delegator", delegator);
                        session.setAttribute("security", security);
                        session.setAttribute("authz", authz);
                    }
        

        But yeah, all these code may be ignored if you find a mechanism to remove/add before/after serialization/deserialization these objects: this would be the right way to fix the issue.

        Show
        Jacopo Cappellato added a comment - - edited Jacques, am I wrong orin this last patch the meaning of the distributableApplication field is completely inverted? I also still see the pattern: if (!variable) { // false } else {// true } rather than the easier to read: if (variable) { // true } else {// false } Also, but this is a minor note, when I suggested to group together all the code related to distributable apple I was thinking to refactor the code in this way: Security security = null ; try { security = SecurityFactory.getInstance(delegator); } catch (SecurityConfigurationException e) { Debug.logError(e, module); } Authorization authz = null ; try { authz = AuthorizationFactory.getInstance(delegator); } catch (SecurityConfigurationException e) { Debug.logError(e, module); } // set all request attributes here: request.setAttribute( "dispatcher" , dispatcher); request.setAttribute( "delegator" , delegator); request.setAttribute( "security" , security); request.setAttribute( "authz" , authz); // set all session attributes here: if (distributableApplication) { // these attributes are nulled here because they are not serializable and then recreated when the session is deserialized session.setAttribute( "dispatcher" , null ); session.setAttribute( "delegator" , null ); session.setAttribute( "security" , null ); session.setAttribute( "authz" , null ); } else { session.setAttribute( "dispatcher" , dispatcher); session.setAttribute( "delegator" , delegator); session.setAttribute( "security" , security); session.setAttribute( "authz" , authz); } But yeah, all these code may be ignored if you find a mechanism to remove/add before/after serialization/deserialization these objects: this would be the right way to fix the issue.
        Hide
        Jacques Le Roux added a comment -

        OK, here is the plan:

        from the delegatorName sets in session

        session.setAttribute("delegatorName", delegatorName);

        we can easily restitute, the 4 variables which are the problem:

        // String delegatorName = session.getAttribute("delegatorName"); // set above, so always present
        // Delegator delegator = DelegatorFactory.getDelegator(delegatorName);
        // LocalDispatcher dispatcher = ContextFilter.makeWebappDispatcher(session.getServletContext(), delegator);
        // Security security = SecurityFactory.getInstance(delegator)
        // Authorization authz = AuthorizationFactory.getInstance(delegator)

        I checked session.getAttribute() for each names in {.java, *.groovy}, sessionAttributes. in .ftl, and in *een.xml for the names themself, they are needed in

        1. ControlServlet.doGet() (the 4)
        2. BirtWorker.setWebContextObjects() (delegator, dispatcher, security)
        3. ScrumEvents.timeSheetChecker() (delegator)

        In ControlServlet.doGet(), getServletContext().getAttribute() is used to retrieve the 4 if missing => not a pb. Same for the others, if ever it's one then we can still use the lines of the snippet above in due places. I will not fix all OFBiz code for this issue, for instance I read in ScrumEvents.timeSheetChecker()

                Delegator delegator = (Delegator) session.getAttribute("delegator");
                [...]
                if (UtilValidate.isEmpty(delegator)) {
                    delegator = (Delegator) request.getAttribute("delegator");
                }
        

        Why not? (I guess it's C/P not refactored)

                Delegator delegator = (Delegator) request.getAttribute("delegator");
        

        etc.

        So I believe we simply have to not put them in session when using distributable applications, see the attached patch . I refactored for easier/safer reading, and yes reversed the condition. I finally really wonder if they are usefull in session at all. I put a NOTE2 under the NOTE:
        // NOTE: we do NOT want to set this in the servletContext, only in the request and session
        // NOTE2: it would have been more helpful to know the why than the what (only few words)!

        Show
        Jacques Le Roux added a comment - OK, here is the plan: from the delegatorName sets in session session.setAttribute("delegatorName", delegatorName); we can easily restitute, the 4 variables which are the problem: // String delegatorName = session.getAttribute("delegatorName"); // set above, so always present // Delegator delegator = DelegatorFactory.getDelegator(delegatorName); // LocalDispatcher dispatcher = ContextFilter.makeWebappDispatcher(session.getServletContext(), delegator); // Security security = SecurityFactory.getInstance(delegator) // Authorization authz = AuthorizationFactory.getInstance(delegator) I checked session.getAttribute() for each names in { .java, *.groovy}, sessionAttributes. in .ftl, and in *een .xml for the names themself, they are needed in ControlServlet.doGet() (the 4) BirtWorker.setWebContextObjects() (delegator, dispatcher, security) ScrumEvents.timeSheetChecker() (delegator) In ControlServlet.doGet(), getServletContext().getAttribute() is used to retrieve the 4 if missing => not a pb. Same for the others, if ever it's one then we can still use the lines of the snippet above in due places. I will not fix all OFBiz code for this issue, for instance I read in ScrumEvents.timeSheetChecker() Delegator delegator = (Delegator) session.getAttribute( "delegator" ); [...] if (UtilValidate.isEmpty(delegator)) { delegator = (Delegator) request.getAttribute( "delegator" ); } Why not? (I guess it's C/P not refactored) Delegator delegator = (Delegator) request.getAttribute( "delegator" ); etc. So I believe we simply have to not put them in session when using distributable applications, see the attached patch . I refactored for easier/safer reading, and yes reversed the condition. I finally really wonder if they are usefull in session at all. I put a NOTE2 under the NOTE: // NOTE: we do NOT want to set this in the servletContext, only in the request and session // NOTE2: it would have been more helpful to know the why than the what (only few words)!
        Hide
        Jacques Le Roux added a comment -

        OK about the NOTE and NOTE2. I did a small research this morning and as I thought (I certainly did the same intially one year ago, but it was blured in my mind) it's related to multitenant. Look for setWebContextObjects at r927271

        I still believe the session variables are useless. To be sure we have to test my changes in multitenant with distributed applications.

        Show
        Jacques Le Roux added a comment - OK about the NOTE and NOTE2. I did a small research this morning and as I thought (I certainly did the same intially one year ago, but it was blured in my mind) it's related to multitenant. Look for setWebContextObjects at r927271 I still believe the session variables are useless. To be sure we have to test my changes in multitenant with distributed applications.
        Hide
        Jacques Le Roux added a comment -

        Ha this comment is interesting

                    if (setupNewDelegatorEtc) {
                        // now set the delegator and dispatcher in a bunch of places just in case they were changed
                        setWebContextObjects(request, response, delegator, dispatcher);
                    }
        
        Show
        Jacques Le Roux added a comment - Ha this comment is interesting if (setupNewDelegatorEtc) { // now set the delegator and dispatcher in a bunch of places just in case they were changed setWebContextObjects(request, response, delegator, dispatcher); }
        Hide
        Jacopo Cappellato added a comment -

        Jacques,

        there are so many things that I still don't like in your patch, I am sorry (if you think I am becoming too picky, please ask for the review of someone else, it would actually be good to get others' opinion).

        A) this should be logged as info rather than error (deploying on tomcat is not mandatory):

        +                    Debug.logError(e, "No catalina-container configuration found in container config!");

        B)
        I am still confused by the usage of the usage of the "distributableApplication" variable; the way I understand its meaning is the following:

        • distributableApplication should match the value of the xml field "apps-distributable"
        • when this value is set to true then the application will be distributed and the sessions will be serialized

        If the two sentences are correct then there are issues in your code.

        C)
        And your last patch is, by default, not setting the fields in session and this will again generate the error that I fixed a few days ago (introduced when you first implemented this feature).

        D)
        The steps to recreate the error are listed in a previous comment in this task... if you simply set to null (as you still are suggesting) the fields when apps-distributable is set to true then the error will happen again so your proposed patch doesn't fix the issue.

        E)
        A final note; what is the purpose of this comment:

        +        // NOTE2: it would have been more helpful to know the why than the what (only few words)!

        ? I hope you are not proposing to commit it

        Show
        Jacopo Cappellato added a comment - Jacques, there are so many things that I still don't like in your patch, I am sorry (if you think I am becoming too picky, please ask for the review of someone else, it would actually be good to get others' opinion). A) this should be logged as info rather than error (deploying on tomcat is not mandatory): + Debug.logError(e, "No catalina-container configuration found in container config!" ); B) I am still confused by the usage of the usage of the "distributableApplication" variable; the way I understand its meaning is the following: distributableApplication should match the value of the xml field "apps-distributable" when this value is set to true then the application will be distributed and the sessions will be serialized If the two sentences are correct then there are issues in your code. C) And your last patch is, by default, not setting the fields in session and this will again generate the error that I fixed a few days ago (introduced when you first implemented this feature). D) The steps to recreate the error are listed in a previous comment in this task... if you simply set to null (as you still are suggesting) the fields when apps-distributable is set to true then the error will happen again so your proposed patch doesn't fix the issue. E) A final note; what is the purpose of this comment: + // NOTE2: it would have been more helpful to know the why than the what (only few words)! ? I hope you are not proposing to commit it
        Hide
        Jacques Le Roux added a comment -

        Jacopo,

        A) Right, I will use <<Debug.logError(e, module);>> rather. Note that I don't know yet how this will work when out of the Tomcat context for distributed application. Because, at least for now, I have no means to know if we are in such a context for other application servers. The problem will be the same than with current code: those application servers will try to persist the session and will stumble upon those not serialised variables and a NPE will be thrown. This should be investigated in appserver component, another task...
        B) Right, when you suggested to change the name of the distributableApplication boolean var (from sessionPersistence) it also reversed its meaning. I forgot then to change the initialisation and to remove the negation when assigning it

        Boolean distributableApplication = true;
        [...]
        distributableApplication = !ContainerConfig.getPropertyValue(cc, "apps-distributable", false);
        

        are now

        Boolean distributableApplication = false;
        [...]
        distributableApplication = ContainerConfig.getPropertyValue(cc, "apps-distributable", false);
        

        C) This was due to B. With attached patch, the vars are not put in session only in distributable mode. So current behaviour will continue, but in case of distrib apps we will not cross a NPE.
        D) This was due to B, I tested it's OK with attached patch
        E) Actually the comment I found

        // now set the delegator and dispatcher in a bunch of places just in case they were changed
        

        answers to this question. I have been positive and reused this knowledge in NOTE2 rather than complaining

        I think my last patch answers to all your questions. It works in both cases: apps-distributable being true or false

        The bright side of this brainstorming is that it's now really easy to test. The confusion I intially introduced with DeltaManager no longer exists. And since the default Manager (StandardManager) is also able to persist sessions only changing the value of apps-distributable to true or false allows to test the "feature"

        Just curious: why did you not use the Jira automatic numbering (ie #) and used instead A), B) etc. ?

        Thanks for your continued reviews!

        Show
        Jacques Le Roux added a comment - Jacopo, A) Right, I will use <<Debug.logError(e, module);>> rather. Note that I don't know yet how this will work when out of the Tomcat context for distributed application. Because, at least for now, I have no means to know if we are in such a context for other application servers. The problem will be the same than with current code: those application servers will try to persist the session and will stumble upon those not serialised variables and a NPE will be thrown. This should be investigated in appserver component, another task... B) Right, when you suggested to change the name of the distributableApplication boolean var (from sessionPersistence) it also reversed its meaning. I forgot then to change the initialisation and to remove the negation when assigning it Boolean distributableApplication = true ; [...] distributableApplication = !ContainerConfig.getPropertyValue(cc, "apps-distributable" , false ); are now Boolean distributableApplication = false ; [...] distributableApplication = ContainerConfig.getPropertyValue(cc, "apps-distributable" , false ); C) This was due to B. With attached patch, the vars are not put in session only in distributable mode. So current behaviour will continue, but in case of distrib apps we will not cross a NPE. D) This was due to B, I tested it's OK with attached patch E) Actually the comment I found // now set the delegator and dispatcher in a bunch of places just in case they were changed answers to this question. I have been positive and reused this knowledge in NOTE2 rather than complaining I think my last patch answers to all your questions. It works in both cases: apps-distributable being true or false The bright side of this brainstorming is that it's now really easy to test. The confusion I intially introduced with DeltaManager no longer exists. And since the default Manager (StandardManager) is also able to persist sessions only changing the value of apps-distributable to true or false allows to test the "feature" Just curious: why did you not use the Jira automatic numbering (ie #) and used instead A), B) etc. ? Thanks for your continued reviews!
        Hide
        Jacopo Cappellato added a comment -

        Jacques... there are still issues and I am running out of time

        • if you use Debug.logError(e, module), all deployments using Jetty will get an error logged; please use logInfo instead as I suggested earlier
        • the default method calls are still all wrong:
          setWebContextObjects(request, response, delegator, dispatcher, true);

          should be:

          setWebContextObjects(request, response, delegator, dispatcher, false);
        • and maybe I was not clear enough but I don't see how the issue that I reported with testing steps (login/logout/login with different account/visit another web app) could be fixed (for distributed applications) by your patch
        Show
        Jacopo Cappellato added a comment - Jacques... there are still issues and I am running out of time if you use Debug.logError(e, module), all deployments using Jetty will get an error logged; please use logInfo instead as I suggested earlier the default method calls are still all wrong: setWebContextObjects(request, response, delegator, dispatcher, true ); should be: setWebContextObjects(request, response, delegator, dispatcher, false ); and maybe I was not clear enough but I don't see how the issue that I reported with testing steps (login/logout/login with different account/visit another web app) could be fixed (for distributed applications) by your patch
        Hide
        Jacques Le Roux added a comment -

        Jacopo,

        1. OK, easy fix
        2. Indeed missed those also when changing the name, fixed in last attached patch
        3. Did you test? I did and it works in both cases, I also wrote above how to test, simply:

          change the value of apps-distributable to true or false allows to test the "feature"

        There is no hurry, just that current code does not work with distributable applications. Because the app server will try to persist those 4 variables (1st one will generate a blocking NPE)

        Show
        Jacques Le Roux added a comment - Jacopo, OK, easy fix Indeed missed those also when changing the name, fixed in last attached patch Did you test? I did and it works in both cases, I also wrote above how to test, simply: change the value of apps-distributable to true or false allows to test the "feature" There is no hurry, just that current code does not work with distributable applications. Because the app server will try to persist those 4 variables (1st one will generate a blocking NPE)
        Hide
        Jacopo Cappellato added a comment -

        I tested it and I see errors in the log (cause by delegator not being serializable) and this is actually what I was expecting since you call

        setWebContextObjects(request, response, delegator, dispatcher, false);

        at login and you only check apps-distributable at logout... I don't see how this could work for you.

        But as I said several times, the idea of the fix is wrong at least until you figure out why these attributes are set in the session (are they used somewhere? is it to improve performance? other reasons?): you cannot fix it by removing the values and not taking care of providing a mechanism to restore them without causing too much overhead when the session is deserialized (and the comments you added simply indicate that some ootb code could be broken but they don't fix the issue).

        Am I the only one who thinks that this is wrong way to tackle the problem?

        Show
        Jacopo Cappellato added a comment - I tested it and I see errors in the log (cause by delegator not being serializable) and this is actually what I was expecting since you call setWebContextObjects(request, response, delegator, dispatcher, false ); at login and you only check apps-distributable at logout... I don't see how this could work for you. But as I said several times, the idea of the fix is wrong at least until you figure out why these attributes are set in the session (are they used somewhere? is it to improve performance? other reasons?): you cannot fix it by removing the values and not taking care of providing a mechanism to restore them without causing too much overhead when the session is deserialized (and the comments you added simply indicate that some ootb code could be broken but they don't fix the issue). Am I the only one who thinks that this is wrong way to tackle the problem?
        Hide
        Jacopo Cappellato added a comment -

        With reference to the error in the logs I reported in my last comment: it could be caused by a session object created before the code change.
        I am reviewing the ootb code to see if we can get rid of a few old code.

        Show
        Jacopo Cappellato added a comment - With reference to the error in the logs I reported in my last comment: it could be caused by a session object created before the code change. I am reviewing the ootb code to see if we can get rid of a few old code.
        Hide
        Jacopo Cappellato added a comment -

        No, the error is actually:

        2012-06-25 12:55:34,799 (0.0.0.0-startStop-1) [ StandardSession.java:1677:WARN ] Cannot serialize session attribute LAST_VIEW_PARAMS for session 22BA02AF26EA4852DB8C79CF0052F22B.jvm1
        java.io.NotSerializableException: org.ofbiz.entity.GenericDelegator
        at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1164)
        at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1518)
        at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1483)
        at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1400)
        at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1158)
        at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:330)
        at javolution.util.FastMap.writeObject(FastMap.java:1513)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)
        at java.io.ObjectStreamClass.invokeWriteObject(ObjectStreamClass.java:940)
        at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1469)
        at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1400)
        at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1158)
        at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:330)
        at org.apache.catalina.session.StandardSession.writeObject(StandardSession.java:1671)
        at org.apache.catalina.session.StandardSession.writeObjectData(StandardSession.java:1077)
        at org.apache.catalina.session.StandardManager.doUnload(StandardManager.java:432)
        at org.apache.catalina.session.StandardManager.unload(StandardManager.java:353)
        at org.apache.catalina.session.StandardManager.stopInternal(StandardManager.java:518)
        at org.apache.catalina.util.LifecycleBase.stop(LifecycleBase.java:232)
        at org.apache.catalina.core.StandardContext.stopInternal(StandardContext.java:5473)
        at org.apache.catalina.util.LifecycleBase.stop(LifecycleBase.java:232)
        at org.apache.catalina.core.ContainerBase$StopChild.call(ContainerBase.java:1611)
        at org.apache.catalina.core.ContainerBase$StopChild.call(ContainerBase.java:1600)
        at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
        at java.util.concurrent.FutureTask.run(FutureTask.java:138)
        at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
        at java.lang.Thread.run(Thread.java:680)

        Show
        Jacopo Cappellato added a comment - No, the error is actually: 2012-06-25 12:55:34,799 (0.0.0.0-startStop-1) [ StandardSession.java:1677:WARN ] Cannot serialize session attribute LAST_VIEW_PARAMS for session 22BA02AF26EA4852DB8C79CF0052F22B.jvm1 java.io.NotSerializableException: org.ofbiz.entity.GenericDelegator at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1164) at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1518) at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1483) at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1400) at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1158) at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:330) at javolution.util.FastMap.writeObject(FastMap.java:1513) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at java.io.ObjectStreamClass.invokeWriteObject(ObjectStreamClass.java:940) at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1469) at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1400) at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1158) at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:330) at org.apache.catalina.session.StandardSession.writeObject(StandardSession.java:1671) at org.apache.catalina.session.StandardSession.writeObjectData(StandardSession.java:1077) at org.apache.catalina.session.StandardManager.doUnload(StandardManager.java:432) at org.apache.catalina.session.StandardManager.unload(StandardManager.java:353) at org.apache.catalina.session.StandardManager.stopInternal(StandardManager.java:518) at org.apache.catalina.util.LifecycleBase.stop(LifecycleBase.java:232) at org.apache.catalina.core.StandardContext.stopInternal(StandardContext.java:5473) at org.apache.catalina.util.LifecycleBase.stop(LifecycleBase.java:232) at org.apache.catalina.core.ContainerBase$StopChild.call(ContainerBase.java:1611) at org.apache.catalina.core.ContainerBase$StopChild.call(ContainerBase.java:1600) at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303) at java.util.concurrent.FutureTask.run(FutureTask.java:138) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908) at java.lang.Thread.run(Thread.java:680)
        Hide
        Jacques Le Roux added a comment - - edited

        == TYPO ==
        I get this also (in a customised instance but not much changed in framework)
        2012-06-12 10:35:29,921 (OFBiz_Shutdown_Hook) [ StandardSession.java:1567:WARN ] Cannot serialize session attribute LAST_VIEW_PARAMS for session 00C48D0D76821C98403482FAE8B77C24.jvm1
        java.io.NotSerializableException: net.sf.json.JSONObject
        at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1164)
        at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:330)
        at javolution.util.FastMap.writeObject(FastMap.java:1513)

        Do LAST_VIEW_PARAMS is a FastMap<String, Object> returned by UtilHttp.getParameterMap(). I read it implements javolution.xml.XMLSerializable but not sure what this is and the Tomcat StandarManager does not seems to appreciate (StandardSession.java:1567:WARN). We can replace by an HashMap which implements Serializable

        Index: framework/base/src/org/ofbiz/base/util/UtilHttp.java
        ===================================================================
        --- framework/base/src/org/ofbiz/base/util/UtilHttp.java	(revision 1353393)
        +++ framework/base/src/org/ofbiz/base/util/UtilHttp.java	(working copy)
        @@ -113,7 +113,7 @@
              */
             public static Map<String, Object> getParameterMap(HttpServletRequest request, Set<? extends String> nameSet, Boolean onlyIncludeOrSkip) {
                 boolean onlyIncludeOrSkipPrim = onlyIncludeOrSkip == null ? true : onlyIncludeOrSkip.booleanValue();
        -        Map<String, Object> paramMap = FastMap.newInstance();
        +        Map<String, Object> paramMap = new HashMap<String, Object>();
         
                 // add all the actual HTTP request parameters
                 Enumeration<String> e = UtilGenerics.cast(request.getParameterNames());
        

        I tried it seems to work fine (small test, see attached log)

        Note that LAST_VIEW_PARAMS was introduced previously to multitenant changes. There are maybe other such Objects put in session, but (quickly) looking around in RequestHandler found none (others are simple String).

        Show
        Jacques Le Roux added a comment - - edited == TYPO == I get this also (in a customised instance but not much changed in framework) 2012-06-12 10:35:29,921 (OFBiz_Shutdown_Hook) [ StandardSession.java:1567:WARN ] Cannot serialize session attribute LAST_VIEW_PARAMS for session 00C48D0D76821C98403482FAE8B77C24.jvm1 java.io.NotSerializableException: net.sf.json.JSONObject at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1164) at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:330) at javolution.util.FastMap.writeObject(FastMap.java:1513) Do LAST_VIEW_PARAMS is a FastMap<String, Object> returned by UtilHttp.getParameterMap(). I read it implements javolution.xml.XMLSerializable but not sure what this is and the Tomcat StandarManager does not seems to appreciate (StandardSession.java:1567:WARN). We can replace by an HashMap which implements Serializable Index: framework/base/src/org/ofbiz/base/util/UtilHttp.java =================================================================== --- framework/base/src/org/ofbiz/base/util/UtilHttp.java (revision 1353393) +++ framework/base/src/org/ofbiz/base/util/UtilHttp.java (working copy) @@ -113,7 +113,7 @@ */ public static Map< String , Object > getParameterMap(HttpServletRequest request, Set<? extends String > nameSet, Boolean onlyIncludeOrSkip) { boolean onlyIncludeOrSkipPrim = onlyIncludeOrSkip == null ? true : onlyIncludeOrSkip.booleanValue(); - Map< String , Object > paramMap = FastMap.newInstance(); + Map< String , Object > paramMap = new HashMap< String , Object >(); // add all the actual HTTP request parameters Enumeration< String > e = UtilGenerics. cast (request.getParameterNames()); I tried it seems to work fine (small test, see attached log) Note that LAST_VIEW_PARAMS was introduced previously to multitenant changes. There are maybe other such Objects put in session, but (quickly) looking around in RequestHandler found none (others are simple String).
        Hide
        Jacopo Cappellato added a comment -

        The way I see it is that the map in the session may contain non serializable objects like for example the Delegator or a JSONObject object.

        Show
        Jacopo Cappellato added a comment - The way I see it is that the map in the session may contain non serializable objects like for example the Delegator or a JSONObject object.
        Hide
        Jacques Le Roux added a comment -

        Yes, I tried to fix an issue I crossed because it was blocking. For the others it depends on the needs, in the eCommerce world, round robin LB and sticky sessions do it, you rarely need to really persist sessions.

        Show
        Jacques Le Roux added a comment - Yes, I tried to fix an issue I crossed because it was blocking . For the others it depends on the needs, in the eCommerce world, round robin LB and sticky sessions do it, you rarely need to really persist sessions.
        Hide
        Jacopo Cappellato added a comment -

        In short, this fix really covers a small part of the issues you will experience if you set the "apps-distributable" to true: specifically just one you found in a very specific scenario; in general I would discourage to use the apps-distributable flag in production unless you really know what will go into the session. Based on this I don't think it is worth of adding special handling for this scenario to the framework as proposed in this ticket; by the way I will try to clean up a few objects currently set in the session, I will probably start a conversation on this in the dev list.

        Show
        Jacopo Cappellato added a comment - In short, this fix really covers a small part of the issues you will experience if you set the "apps-distributable" to true: specifically just one you found in a very specific scenario; in general I would discourage to use the apps-distributable flag in production unless you really know what will go into the session. Based on this I don't think it is worth of adding special handling for this scenario to the framework as proposed in this ticket; by the way I will try to clean up a few objects currently set in the session, I will probably start a conversation on this in the dev list.
        Hide
        Jacques Le Roux added a comment -

        I agree Jacopo

        Show
        Jacques Le Roux added a comment - I agree Jacopo
        Hide
        Jacques Le Roux added a comment -

        One last note though: what I put in my last patch is much better than what we have in trunk currenlty. It's not complete, but it's correct at least...

        Show
        Jacques Le Roux added a comment - One last note though: what I put in my last patch is much better than what we have in trunk currenlty. It's not complete, but it's correct at least...
        Hide
        Jacopo Cappellato added a comment -

        It should be fixed in rev. 1353681
        Please resolve the issue if it works also for you.

        Show
        Jacopo Cappellato added a comment - It should be fixed in rev. 1353681 Please resolve the issue if it works also for you.
        Hide
        Jacques Le Roux added a comment -

        Jacopo,

        I reviewed and it's ok with me, much simpler no longer need to muck around with apps-distributable in LoginWorker. Of course I briefly tested in both cases and it works. Not surprising, since we have no longer these 4 non serialised variables in session.

        One more thing I'd do though: replace FastMap by HashMap in UtilHttp.getParameterMap(). Because this is still used in at least LoginWorker.checkLogin() to put PREVIOUS_PARAM_MAP_FORM insession, see this snippet

        Map<String, Object> formParams = UtilHttp.getParameterMap(request, urlParams.keySet(), false);
        if (UtilValidate.isNotEmpty(formParams)) {
            session.setAttribute("_PREVIOUS_PARAM_MAP_FORM_", formParams);
        }
        

        And morevoer it could be used in other cases by not aware developers. My patch above can be used, not sure why you did not, so I don't commit...

        Show
        Jacques Le Roux added a comment - Jacopo, I reviewed and it's ok with me, much simpler no longer need to muck around with apps-distributable in LoginWorker. Of course I briefly tested in both cases and it works. Not surprising, since we have no longer these 4 non serialised variables in session. One more thing I'd do though: replace FastMap by HashMap in UtilHttp.getParameterMap(). Because this is still used in at least LoginWorker.checkLogin() to put PREVIOUS_PARAM_MAP_FORM insession, see this snippet Map< String , Object > formParams = UtilHttp.getParameterMap(request, urlParams.keySet(), false ); if (UtilValidate.isNotEmpty(formParams)) { session.setAttribute( "_PREVIOUS_PARAM_MAP_FORM_" , formParams); } And morevoer it could be used in other cases by not aware developers. My patch above can be used, not sure why you did not, so I don't commit...
        Hide
        Jacques Le Roux added a comment -

        Before closing I'd like to backport (maybe partially) this at least in R10, 11 & 12 branches. Not sure why you did not, so I wait...

        Show
        Jacques Le Roux added a comment - Before closing I'd like to backport (maybe partially) this at least in R10, 11 & 12 branches. Not sure why you did not, so I wait...
        Hide
        Jacopo Cappellato added a comment -

        Please feel free to back port (maybe wait a few to get some feedback from developers active in multi tenant implementation); as regards the FastMap... I am not against the switch to HashMap but I don't think it is relevant here because FastMap implements XMLSerializable that extends Serializable.

        Show
        Jacopo Cappellato added a comment - Please feel free to back port (maybe wait a few to get some feedback from developers active in multi tenant implementation); as regards the FastMap... I am not against the switch to HashMap but I don't think it is relevant here because FastMap implements XMLSerializable that extends Serializable.
        Hide
        Jacques Le Roux added a comment -

        Sure I will wait maybe more, no hurry there.

        Ha, I did not check deeper on XMLSerializable, but then why do we get error like I posted above, ie:

        2012-06-12 10:35:29,921 (OFBiz_Shutdown_Hook) [ StandardSession.java:1567:WARN ] Cannot serialize session attribute LAST_VIEW_PARAMS for session 00C48D0D76821C98403482FAE8B77C24.jvm1
        java.io.NotSerializableException: net.sf.json.JSONObject
        at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1164)
        at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:330)
        at javolution.util.FastMap.writeObject(FastMap.java:1513)

        This is resolved with HashMap, weird then isn'it?

        Show
        Jacques Le Roux added a comment - Sure I will wait maybe more, no hurry there. Ha, I did not check deeper on XMLSerializable, but then why do we get error like I posted above, ie: 2012-06-12 10:35:29,921 (OFBiz_Shutdown_Hook) [ StandardSession.java:1567:WARN ] Cannot serialize session attribute LAST_VIEW_PARAMS for session 00C48D0D76821C98403482FAE8B77C24.jvm1 java.io.NotSerializableException: net.sf.json.JSONObject at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1164) at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:330) at javolution.util.FastMap.writeObject(FastMap.java:1513) This is resolved with HashMap, weird then isn'it?
        Hide
        Jacopo Cappellato added a comment -

        I suspect that it worked because of the restart and the actions you did (for example you didn't visit a screen with Ajax stuff).

        Show
        Jacopo Cappellato added a comment - I suspect that it worked because of the restart and the actions you did (for example you didn't visit a screen with Ajax stuff).
        Hide
        Jacques Le Roux added a comment -

        I got to the Catalog main page where Ajax is used. But actually the issue above appears when you stop the application. If distributed apps is used the Tomcat Manager will try to serialise the session. If you look at the log I posted you will see this error does not appear then.

        Show
        Jacques Le Roux added a comment - I got to the Catalog main page where Ajax is used. But actually the issue above appears when you stop the application. If distributed apps is used the Tomcat Manager will try to serialise the session. If you look at the log I posted you will see this error does not appear then.
        Hide
        Jacques Le Roux added a comment -

        I fixed the FastMap issue in
        trunk r1355660
        R12.04 r1355661
        R11.04 r1355662
        R10.04 r1355663

        Show
        Jacques Le Roux added a comment - I fixed the FastMap issue in trunk r1355660 R12.04 r1355661 R11.04 r1355662 R10.04 r1355663
        Hide
        Jacques Le Roux added a comment -

        Note: to test the above you need to set apps-distributable to true and put some <distributable/> in the web.xml files of the applications to test

        BTW I found this (unrelated) error when shutting down OFBIz
        2012-06-30 10:21:22,125 (0.0.0.0-startStop-1) [ WebappClassLoader.java:2269:ERROR] The web application [/ecommerce] appears to have started a thread named [ReferenceCleaner] but has failed to stop it. This is very likely to create a memory leak.

        Show
        Jacques Le Roux added a comment - Note: to test the above you need to set apps-distributable to true and put some <distributable/> in the web.xml files of the applications to test BTW I found this (unrelated) error when shutting down OFBIz 2012-06-30 10:21:22,125 (0.0.0.0-startStop-1) [ WebappClassLoader.java:2269:ERROR] The web application [/ecommerce] appears to have started a thread named [ReferenceCleaner] but has failed to stop it. This is very likely to create a memory leak.
        Hide
        Jacques Le Roux added a comment -

        In R12.04, I backported r1353135 at r1358544 and r1353681 at r1358545
        The 2 others releases branches (11 and 10) had too much conflicts, I gave up

        Show
        Jacques Le Roux added a comment - In R12.04, I backported r1353135 at r1358544 and r1353681 at r1358545 The 2 others releases branches (11 and 10) had too much conflicts, I gave up

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Jacques Le Roux
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development