Tapestry 5
  1. Tapestry 5
  2. TAP5-1355

Threading issue with SessionStateObjects

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.2.4
    • Fix Version/s: 5.3
    • Component/s: tapestry-core
    • Labels:
      None

      Description

      When a page request consists of multiple HTTP request (e.g. page and some dynamically generated images) and all those requests access a SessionStateObject, it happens that a new session (with an empty SSO) is created for some of the request threads.

      I was able to create a very simple example to recreate that problem:

      -A simple page that displays 20 dynamically generated images in a loop.
      -In the page, a SSO, holding a number value is initialized to a random number.
      -Each of the dynamic images read that number and draws it.
      -Make sure that a HTTP-Request is made for every image on the page (by adding some random number to the event link)

      The effect that you'll see after some reloads of the page (maybe you need to reload 30 times) is that some images will draw 0 as SSO value instead of the number set in the page @BeginRender method. Those fields will be marked in red in the demo so you can quickly see them.

      I definitely beleive that tapestry should take care of this. It is a use case for SSOs that is probably too common to ignore.

      Why can't this be automatically integrated into the ApplicationStateManager?

      The demo has been deployed here

      http://www.avetana.de/taptest/

      1. Screenshot.png.jpg
        48 kB
        Moritz Gmelin
      2. taptest.tgz
        4 kB
        Moritz Gmelin

        Issue Links

          Activity

          Moritz Gmelin created issue -
          Moritz Gmelin made changes -
          Field Original Value New Value
          Attachment taptest.tgz [ 12460346 ]
          Moritz Gmelin made changes -
          Description When a page request consists of multiple HTTP request (e.g. page and some dynamically generated images) and all those requests access a SessionStateObject, it happens that a new session (with an empty SSO) is created for some of the request threads.

          I was able to create a very simple example to recreate that problem:

          -A simple page that displays 20 dynamically generated images in a loop.
          -In the page, a SSO, holding a number value is initialized to a random number.
          -Each of the dynamic images read that number and draws it.
          -Make sure that a HTTP-Request is made for every image on the page (by adding some random number to the event link)

          The effect that you'll see after some reloads of the page (maybe you need to reload 30 times) is that some images will draw 0 as SSO value instead of the number set in the page @BeginRender method. Those fields will be marked in red in the demo so you can quickly see them.

          I definitely beleive that tapestry should take care of this. It is a use case for SSOs that is probably too common to ignore.

          Why can't this be automatically integrated into the ApplicationStateManager?
           
          When a page request consists of multiple HTTP request (e.g. page and some dynamically generated images) and all those requests access a SessionStateObject, it happens that a new session (with an empty SSO) is created for some of the request threads.

          I was able to create a very simple example to recreate that problem:

          -A simple page that displays 20 dynamically generated images in a loop.
          -In the page, a SSO, holding a number value is initialized to a random number.
          -Each of the dynamic images read that number and draws it.
          -Make sure that a HTTP-Request is made for every image on the page (by adding some random number to the event link)

          The effect that you'll see after some reloads of the page (maybe you need to reload 30 times) is that some images will draw 0 as SSO value instead of the number set in the page @BeginRender method. Those fields will be marked in red in the demo so you can quickly see them.

          I definitely beleive that tapestry should take care of this. It is a use case for SSOs that is probably too common to ignore.

          Why can't this be automatically integrated into the ApplicationStateManager?
           
          The demo has been deployed here

          http://www.avetana.de/taptest/
          Hide
          Moritz Gmelin added a comment -

          This is how it looks like when some of the images get a new session

          Show
          Moritz Gmelin added a comment - This is how it looks like when some of the images get a new session
          Moritz Gmelin made changes -
          Attachment Screenshot.png.jpg [ 12460350 ]
          Hide
          Josh Canfield added a comment -

          The problem appears to be with a bug fix for TAP5-834.

          if (analyzer.isDirty(attributeValue))

          { // TAP5-834: Jetty & Tomcat work by object identity, will not update the attribute // and fire the session binding event unless there's a real change. So we set the // attribute to null and then to the new value and that should force the necessary // notification. session.setAttribute(attributeName, null); session.setAttribute(attributeName, attributeValue); }

          For a moment the session attribute is set to null. Sometimes code asks for the session value before it gets to the next line where it sets it back.

          Show
          Josh Canfield added a comment - The problem appears to be with a bug fix for TAP5-834 . if (analyzer.isDirty(attributeValue)) { // TAP5-834: Jetty & Tomcat work by object identity, will not update the attribute // and fire the session binding event unless there's a real change. So we set the // attribute to null and then to the new value and that should force the necessary // notification. session.setAttribute(attributeName, null); session.setAttribute(attributeName, attributeValue); } For a moment the session attribute is set to null. Sometimes code asks for the session value before it gets to the next line where it sets it back.
          Hide
          Thiago H. de Paula Figueiredo added a comment -

          Hi, Josh!

          Instead of reverting the TAP5-834, which would cause other problems, what about putting that two statements in a synchronized block?

          syncronized (session)

          { session.setAttribute(attributeName, null); session.setAttribute(attributeName, attributeValue); }
          Show
          Thiago H. de Paula Figueiredo added a comment - Hi, Josh! Instead of reverting the TAP5-834 , which would cause other problems, what about putting that two statements in a synchronized block? syncronized (session) { session.setAttribute(attributeName, null); session.setAttribute(attributeName, attributeValue); }
          Josh Canfield made changes -
          Link This issue is broken by TAP5-834 [ TAP5-834 ]
          Hide
          Josh Canfield added a comment -

          We need to find an alternative to setting the session attribute to null.

          I'm thinking about wrapping SSO and Persistent Fields in a class that can be swapped out when the SessionPersistentObjectAnalyzer determines they are dirty. Perhaps a SessionPersistedObject which implements Cloneable with a default implementation that does a shallow clone? If it's not a SPO then I think we just ignore it.

          This doesn't help people who setAttribute directly on the session, does anyone care?

          Show
          Josh Canfield added a comment - We need to find an alternative to setting the session attribute to null. I'm thinking about wrapping SSO and Persistent Fields in a class that can be swapped out when the SessionPersistentObjectAnalyzer determines they are dirty. Perhaps a SessionPersistedObject which implements Cloneable with a default implementation that does a shallow clone? If it's not a SPO then I think we just ignore it. This doesn't help people who setAttribute directly on the session, does anyone care?
          Hide
          Josh Canfield added a comment -

          @Thiago
          Yeah, I realized reverting the other defect was wrong, but I had to pack up my computer and re-locate before I could fix it I un-reopened it and marked this as a regression.

          We could synchronize around the session in both the getAttribute method and the restoreDirtyObjects method so that apps would be blocked from getting any attribute value while it was temporarily null. I'm a little hesitant to do that... more of a gut thing than hard data though. I currently prefer the idea of using a wrapper object, but I could be persuaded.

          Show
          Josh Canfield added a comment - @Thiago Yeah, I realized reverting the other defect was wrong, but I had to pack up my computer and re-locate before I could fix it I un-reopened it and marked this as a regression. We could synchronize around the session in both the getAttribute method and the restoreDirtyObjects method so that apps would be blocked from getting any attribute value while it was temporarily null. I'm a little hesitant to do that... more of a gut thing than hard data though. I currently prefer the idea of using a wrapper object, but I could be persuaded.
          Hide
          Andy Blower added a comment -

          May I ask why my original patch to fix the issue TAP5-834 is not being considered. I was pretty surprised that it was not used in the first place, but the second way of fixing the issue was chosen which had possible concurrency issues. My original words:

          "I think there are two possible solutions to this issue - I prefer the first by a large margin, but both modify the SessionImpl.restoreDirtyObjects() method.

          1) Add a new method to OptimizedSessionPersistedObject interface to reset the dirty flag, and a corresponding method in SessionPersistedObjectAnalyzer - implementing them as appropriate, then call the new reset method after setting the session attribute in SessionImpl.restoreDirtyObjects().

          2) Remove the session attribute before adding it in SessionImpl.restoreDirtyObjects(). Although I have a worry that this may potentially cause hard to find concurrency problems."

          The second solution was used, but the first solution (which my patch implemented) is much better. I know it adds a new method to a public interface, but this is not as much impact as changing or removing an existing method one and in this case is trivial for users to implement. There has certainly been worse backwards compatibility changes made in T5, and the benefit of this simple solution (#1 above) far outweighs the impact IMHO. It's also a lot simpler and cleaner than the solutions proposed here. Also IMHO.

          Just FYI the code from the patch was run in our production environment for several months before we upgraded to Tapestry 5.2 and removed my customizations. Luckily we don't seem to have run into any threading issues, well not that have caused any fuss at least.

          Show
          Andy Blower added a comment - May I ask why my original patch to fix the issue TAP5-834 is not being considered. I was pretty surprised that it was not used in the first place, but the second way of fixing the issue was chosen which had possible concurrency issues. My original words: "I think there are two possible solutions to this issue - I prefer the first by a large margin, but both modify the SessionImpl.restoreDirtyObjects() method. 1) Add a new method to OptimizedSessionPersistedObject interface to reset the dirty flag, and a corresponding method in SessionPersistedObjectAnalyzer - implementing them as appropriate, then call the new reset method after setting the session attribute in SessionImpl.restoreDirtyObjects(). 2) Remove the session attribute before adding it in SessionImpl.restoreDirtyObjects(). Although I have a worry that this may potentially cause hard to find concurrency problems." The second solution was used, but the first solution (which my patch implemented) is much better. I know it adds a new method to a public interface, but this is not as much impact as changing or removing an existing method one and in this case is trivial for users to implement. There has certainly been worse backwards compatibility changes made in T5, and the benefit of this simple solution (#1 above) far outweighs the impact IMHO. It's also a lot simpler and cleaner than the solutions proposed here. Also IMHO. Just FYI the code from the patch was run in our production environment for several months before we upgraded to Tapestry 5.2 and removed my customizations. Luckily we don't seem to have run into any threading issues, well not that have caused any fuss at least.
          Hide
          Josh Canfield added a comment -

          Hi Andy.

          Sorry, I jumped to solving the immediate problem and didn't step back to consider the base problem that was originally being solved. The problem seems to be that Tapestry is depending on HttpSessionBindingListener in order to manage session attribute lifecycle, and that the behavior is container specific. I looked at the Jetty code and it uses .equals, but Tomcat uses != to determine whether to call the valueBound method.

          The servlet spec is not clear about what the right behavior is for the container. I was surprised that the spec is actually self contradictory about how the method was even supposed to be called. For instance, in the 2.4 spec SRV.7.4 "The valueBound method must be called before the object is made available via the getAttribute method of the HttpSession interface." and then in SRV.15.1.7, the javadoc for HttpSession it says "Notifications are sent after the binding methods complete." Both Tomcat and Jetty seem take the approached defined in the HttpSession docs (maybe I'm reading something wrong here.) This means that in a situation where you are accessing the object in a multi-threaded manner you could get the object from the session before your valueBound method has even run. Threading issues have been left up to the developer.

          The biggest hole though, there is no specification as far as I can find as to what it means to "bind" an object to the session. Jetty will call valueBound if the .equals says it's different, Tomcat only if the object has changed (you only bind an object to the session once). Since we have two containers that behave differently we can only imagine that other containers will take alternating approaches.

          To sum up; the prevailing wisdom seems to be that after changing the value within a session attribute that calling session.setAttribute() is enough to get clustered session attribute replicated. Having spent some time with this code I think the dependency on HttpSessionBindingListener should be dropped and Tapestry should go with Andy's number two solution. Keeping the dirty/clean cycle within Tapestry's code.

          Show
          Josh Canfield added a comment - Hi Andy. Sorry, I jumped to solving the immediate problem and didn't step back to consider the base problem that was originally being solved. The problem seems to be that Tapestry is depending on HttpSessionBindingListener in order to manage session attribute lifecycle, and that the behavior is container specific. I looked at the Jetty code and it uses .equals, but Tomcat uses != to determine whether to call the valueBound method. The servlet spec is not clear about what the right behavior is for the container. I was surprised that the spec is actually self contradictory about how the method was even supposed to be called. For instance, in the 2.4 spec SRV.7.4 "The valueBound method must be called before the object is made available via the getAttribute method of the HttpSession interface." and then in SRV.15.1.7, the javadoc for HttpSession it says "Notifications are sent after the binding methods complete." Both Tomcat and Jetty seem take the approached defined in the HttpSession docs (maybe I'm reading something wrong here.) This means that in a situation where you are accessing the object in a multi-threaded manner you could get the object from the session before your valueBound method has even run. Threading issues have been left up to the developer. The biggest hole though, there is no specification as far as I can find as to what it means to "bind" an object to the session. Jetty will call valueBound if the .equals says it's different, Tomcat only if the object has changed (you only bind an object to the session once). Since we have two containers that behave differently we can only imagine that other containers will take alternating approaches. To sum up; the prevailing wisdom seems to be that after changing the value within a session attribute that calling session.setAttribute() is enough to get clustered session attribute replicated. Having spent some time with this code I think the dependency on HttpSessionBindingListener should be dropped and Tapestry should go with Andy's number two solution. Keeping the dirty/clean cycle within Tapestry's code.
          Hide
          Andy Blower added a comment -


          Surely you mean my first solution Josh? The second is essentially what Howard implemented for TAP5-834 and is giving us this problem.

          Show
          Andy Blower added a comment - Surely you mean my first solution Josh? The second is essentially what Howard implemented for TAP5-834 and is giving us this problem.
          Hide
          Josh Canfield added a comment -

          Argh... yes. I'll lie and say that by number two I meant the one that wasn't implemented.

          Show
          Josh Canfield added a comment - Argh... yes. I'll lie and say that by number two I meant the one that wasn't implemented.
          Hide
          Moritz Gmelin added a comment -

          Hi,

          is there any progress on this? Any chance that this will be fixed in 5.2.4.1?

          Moritz

          Show
          Moritz Gmelin added a comment - Hi, is there any progress on this? Any chance that this will be fixed in 5.2.4.1? Moritz
          Josh Canfield made changes -
          Assignee Josh Canfield [ joshcanfield ]
          Josh Canfield made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Hide
          Josh Canfield added a comment -

          SessionPersistedObjectAnalyzer
          Removed isDirty and replaced with checkAndResetDirtyState. Splitting isDirty and reset can result in concurrency problems.

          OptimizedSessionPersistedObject
          Also now has checkAndResetDirtyMarker (hmm inconsistent names)

          BaseOptimizedSessionPersistedObject
          Now uses an AtomicBoolean to track dirty state
          Updated implementers so that they only mark themselves dirty after making changes.

          Show
          Josh Canfield added a comment - SessionPersistedObjectAnalyzer Removed isDirty and replaced with checkAndResetDirtyState. Splitting isDirty and reset can result in concurrency problems. OptimizedSessionPersistedObject Also now has checkAndResetDirtyMarker (hmm inconsistent names) BaseOptimizedSessionPersistedObject Now uses an AtomicBoolean to track dirty state Updated implementers so that they only mark themselves dirty after making changes.
          Josh Canfield made changes -
          Status In Progress [ 3 ] Resolved [ 5 ]
          Fix Version/s 5.3.1 [ 12316673 ]
          Resolution Fixed [ 1 ]
          Howard M. Lewis Ship made changes -
          Fix Version/s 5.3 [ 12316024 ]
          Fix Version/s 5.3.1 [ 12316673 ]
          Howard M. Lewis Ship made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Josh Canfield
              Reporter:
              Moritz Gmelin
            • Votes:
              2 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development