Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 7.8.0
    • Fix Version/s: 6.28.0, 7.9.0, 8.0.0-M8
    • Component/s: wicket
    • Labels:
      None
    • Environment:
      Tomcat

      Description

      WICKET-6387 causes the page store not to be cleared at logout on Tomcat. The problem is that tomcat does not call valueUnbound or valueBound when an attribute is set to the current value (new == old).

      https://github.com/apache/tomcat/blob/e28b35c9e40aeb4b7ac52a98f07ad965630e2766/java/org/apache/catalina/session/StandardSession.java#L1424

      1. WICKET-6465.patch
        5 kB
        Andrea Del Bene

        Issue Links

          Activity

          Hide
          frantam Franta Mejta added a comment -

          Verified in 7.9.0.

          Show
          frantam Franta Mejta added a comment - Verified in 7.9.0.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 8b9e82ffece4e94317b9efcaa5018d6076d86568 in wicket's branch refs/heads/wicket-6.x from Sven Meier
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=8b9e82f ]

          WICKET-6465 prevent unbound during storeTouchedPages with ThreadLocal

          Show
          jira-bot ASF subversion and git services added a comment - Commit 8b9e82ffece4e94317b9efcaa5018d6076d86568 in wicket's branch refs/heads/wicket-6.x from Sven Meier [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=8b9e82f ] WICKET-6465 prevent unbound during storeTouchedPages with ThreadLocal
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 68b24aa788b25a67cfb16bee08a85328dad7b91c in wicket's branch refs/heads/WICKET-6105-java.time from Sven Meier
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=68b24aa ]

          WICKET-6465 prevent unbound during storeTouchedPages with ThreadLocal

          This closes #233

          Show
          jira-bot ASF subversion and git services added a comment - Commit 68b24aa788b25a67cfb16bee08a85328dad7b91c in wicket's branch refs/heads/ WICKET-6105 -java.time from Sven Meier [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=68b24aa ] WICKET-6465 prevent unbound during storeTouchedPages with ThreadLocal This closes #233
          Hide
          svenmeier Sven Meier added a comment -

          With a ThreadLocal the call of #valueUnbound() can now be ignored reliably.

          Show
          svenmeier Sven Meier added a comment - With a ThreadLocal the call of #valueUnbound() can now be ignored reliably.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/wicket/pull/233

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/wicket/pull/233
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 68b24aa788b25a67cfb16bee08a85328dad7b91c in wicket's branch refs/heads/master from Sven Meier
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=68b24aa ]

          WICKET-6465 prevent unbound during storeTouchedPages with ThreadLocal

          This closes #233

          Show
          jira-bot ASF subversion and git services added a comment - Commit 68b24aa788b25a67cfb16bee08a85328dad7b91c in wicket's branch refs/heads/master from Sven Meier [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=68b24aa ] WICKET-6465 prevent unbound during storeTouchedPages with ThreadLocal This closes #233
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit b25984d4e9b9d07c5bf2d3e50c90bbb165554409 in wicket's branch refs/heads/wicket-7.x from Sven Meier
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=b25984d ]

          WICKET-6465 prevent unbound during storeTouchedPages with ThreadLocal

          This closes #233

          Show
          jira-bot ASF subversion and git services added a comment - Commit b25984d4e9b9d07c5bf2d3e50c90bbb165554409 in wicket's branch refs/heads/wicket-7.x from Sven Meier [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=b25984d ] WICKET-6465 prevent unbound during storeTouchedPages with ThreadLocal This closes #233
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bitstorm commented on the issue:

          https://github.com/apache/wicket/pull/233

          Don't forget to fix brackets indentation before commit

          Show
          githubbot ASF GitHub Bot added a comment - Github user bitstorm commented on the issue: https://github.com/apache/wicket/pull/233 Don't forget to fix brackets indentation before commit
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user papegaaij commented on the issue:

          https://github.com/apache/wicket/pull/233

          Yes, this is what I had in mind. I'm ok to merge this in 7.x and 8.

          Show
          githubbot ASF GitHub Bot added a comment - Github user papegaaij commented on the issue: https://github.com/apache/wicket/pull/233 Yes, this is what I had in mind. I'm ok to merge this in 7.x and 8.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user papegaaij commented on the issue:

          https://github.com/apache/wicket/pull/233

          We don't care about concurrent calls to valueUnbound, that's fine. All code in that method is thread safe. What we don't want is multiple calls to storeTouchedPages and valueUnbound (session expiry) to interfere with each other. Synchronizing on the entry will not help as session expiry is probably done from a different thread.

          Show
          githubbot ASF GitHub Bot added a comment - Github user papegaaij commented on the issue: https://github.com/apache/wicket/pull/233 We don't care about concurrent calls to valueUnbound, that's fine. All code in that method is thread safe. What we don't want is multiple calls to storeTouchedPages and valueUnbound (session expiry) to interfere with each other. Synchronizing on the entry will not help as session expiry is probably done from a different thread.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bitstorm commented on the issue:

          https://github.com/apache/wicket/pull/233

          I agree about the use of a normal boolean, but I'm not sure I've understood your idea about ThreadLocal. What I would like to do is to prevent race conditions inside storeTouchedPages. I would do it using synchronized on entry object:

          ```
          protected void storeTouchedPages(final List<IManageablePage> touchedPages)
          {
          if (!touchedPages.isEmpty())
          {
          SessionEntry entry = getSessionEntry(true);
          synchronized (entry)
          {
          entry.setSessionCache(touchedPages);
          for (IManageablePage page : touchedPages)

          { // WICKET-5103 use the same sessionId as used in SessionEntry#getPage() pageStore.storePage(entry.sessionId, page); }

          entry.updating.set(true);
          setSessionAttribute(getAttributeName(), entry);
          }
          }
          }
          ```
          In this way two possible concurrent requests for the same session would execute storeTouchedPages one at a time.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bitstorm commented on the issue: https://github.com/apache/wicket/pull/233 I agree about the use of a normal boolean, but I'm not sure I've understood your idea about ThreadLocal. What I would like to do is to prevent race conditions inside storeTouchedPages. I would do it using synchronized on entry object: ``` protected void storeTouchedPages(final List<IManageablePage> touchedPages) { if (!touchedPages.isEmpty()) { SessionEntry entry = getSessionEntry(true); synchronized (entry) { entry.setSessionCache(touchedPages); for (IManageablePage page : touchedPages) { // WICKET-5103 use the same sessionId as used in SessionEntry#getPage() pageStore.storePage(entry.sessionId, page); } entry.updating.set(true); setSessionAttribute(getAttributeName(), entry); } } } ``` In this way two possible concurrent requests for the same session would execute storeTouchedPages one at a time.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user papegaaij commented on the issue:

          https://github.com/apache/wicket/pull/233

          I don't see why the current implementation uses `AtomicBoolean`. The current implementation could just as well use a normal boolean. If we need to take concurrent access into account (and I think we do), we should probably use a `ThreadLocal`. Of course this assumes that the servlet container will call (un)bound from the same thread, but the current implementation already has that assumption. If a container decides to make the calls async, there is no way of telling if it will happen between the setting and clearing of the boolean.

          Show
          githubbot ASF GitHub Bot added a comment - Github user papegaaij commented on the issue: https://github.com/apache/wicket/pull/233 I don't see why the current implementation uses `AtomicBoolean`. The current implementation could just as well use a normal boolean. If we need to take concurrent access into account (and I think we do), we should probably use a `ThreadLocal`. Of course this assumes that the servlet container will call (un)bound from the same thread, but the current implementation already has that assumption. If a container decides to make the calls async, there is no way of telling if it will happen between the setting and clearing of the boolean.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bitstorm commented on the issue:

          https://github.com/apache/wicket/pull/233

          I'm fine with this quick fix, but I warmly suggest to improve it after it has been applied, at least in master branch.

          About concurrent access afaik we do have to take care of concurrency. At the moment we don't have any guarantee that setSessionAttribute saves the entry for the last request submitted (for the same session).

          Show
          githubbot ASF GitHub Bot added a comment - Github user bitstorm commented on the issue: https://github.com/apache/wicket/pull/233 I'm fine with this quick fix, but I warmly suggest to improve it after it has been applied, at least in master branch. About concurrent access afaik we do have to take care of concurrency. At the moment we don't have any guarantee that setSessionAttribute saves the entry for the last request submitted (for the same session).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user svenmeier commented on the issue:

          https://github.com/apache/wicket/pull/233

          Currently HttpSessionStore and PageStoreManager each use their own instance of HttpSessionBindingListener.
          Yes, we might be able to unify these. But with a simple fix we don't introduce even more - possibly faulty - changes.

          It was unfortunate that we didn't consider all possible callbacks from Servlet containers when this problem started with WICKET-6356.
          IMHO we should go with the simplest fix: Prevent anything bad from happening, when re-setting the session attribute:

          • clustering will work (because the attribute is re-set)
          • it doesn't matter whether the container calls valueBound() or not
          • if the container calls valueUnbound(), it does nothing bad during storeTouchedPages()

          Andrea, what do you think?

          BTW do we have to take care of concurrent access to the SessionEntry? I see that Martin used an AtomicBoolean, but how does that help if there are two request simultaneously storing touched pages?

          Show
          githubbot ASF GitHub Bot added a comment - Github user svenmeier commented on the issue: https://github.com/apache/wicket/pull/233 Currently HttpSessionStore and PageStoreManager each use their own instance of HttpSessionBindingListener. Yes, we might be able to unify these. But with a simple fix we don't introduce even more - possibly faulty - changes. It was unfortunate that we didn't consider all possible callbacks from Servlet containers when this problem started with WICKET-6356 . IMHO we should go with the simplest fix: Prevent anything bad from happening, when re-setting the session attribute: clustering will work (because the attribute is re-set) it doesn't matter whether the container calls valueBound() or not if the container calls valueUnbound(), it does nothing bad during storeTouchedPages() Andrea, what do you think? BTW do we have to take care of concurrent access to the SessionEntry? I see that Martin used an AtomicBoolean, but how does that help if there are two request simultaneously storing touched pages?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bitstorm commented on the issue:

          https://github.com/apache/wicket/pull/233

          I see. thank you for the clarification. Still, I'd like to evaluate more structural solutions, at least for Wicket 8. A good start point might be HttpSessionStore.SessionBindingListener#valueUnbound.
          It's already used to call Session#onInvalidate and we can add Session#clear() after it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bitstorm commented on the issue: https://github.com/apache/wicket/pull/233 I see. thank you for the clarification. Still, I'd like to evaluate more structural solutions, at least for Wicket 8. A good start point might be HttpSessionStore.SessionBindingListener#valueUnbound. It's already used to call Session#onInvalidate and we can add Session#clear() after it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user papegaaij commented on the issue:

          https://github.com/apache/wicket/pull/233

          Indeed, Sesison.destroy() is only called under certain conditions, and in those conditions, valueUnbound() will also be called.

          Show
          githubbot ASF GitHub Bot added a comment - Github user papegaaij commented on the issue: https://github.com/apache/wicket/pull/233 Indeed, Sesison.destroy() is only called under certain conditions, and in those conditions, valueUnbound() will also be called.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user martin-g commented on the issue:

          https://github.com/apache/wicket/pull/233

          When the session expires or is invalidated via API call then `#valueUnbound()` is called and this clears the storages.
          This is how it worked last time I worked in this area of the code.

          Show
          githubbot ASF GitHub Bot added a comment - Github user martin-g commented on the issue: https://github.com/apache/wicket/pull/233 When the session expires or is invalidated via API call then `#valueUnbound()` is called and this clears the storages. This is how it worked last time I worked in this area of the code.
          Hide
          papegaaij Emond Papegaaij added a comment -

          org.apache.wicket.Session#destroy() is only called when a user logs out, not on session timeout. HttpSessionBindingListener.valueUnbound is called on both explicit logout and session timeout. Therefore, the code to clean the disk store should go in valueUnbound, not destroy.

          Show
          papegaaij Emond Papegaaij added a comment - org.apache.wicket.Session#destroy() is only called when a user logs out, not on session timeout. HttpSessionBindingListener.valueUnbound is called on both explicit logout and session timeout. Therefore, the code to clean the disk store should go in valueUnbound, not destroy.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bitstorm commented on the issue:

          https://github.com/apache/wicket/pull/233

          We should consider method Session.destroy() to remove pages when session expires. See my comment at https://issues.apache.org/jira/browse/WICKET-6465

          Show
          githubbot ASF GitHub Bot added a comment - Github user bitstorm commented on the issue: https://github.com/apache/wicket/pull/233 We should consider method Session.destroy() to remove pages when session expires. See my comment at https://issues.apache.org/jira/browse/WICKET-6465
          Hide
          bitstorm Andrea Del Bene added a comment - - edited

          Hi,

          I've noticed something interesting in method Session#destroy that might be part of the problem. As its JavaDoc states the method was meant to dispose session store and page manager when session is invalidated. At the moment there's no code for page manager, it was removed in (massive) commit 4977f66. I would expect that when this method is invoked it calls IPageManager#clear (implemented by PageStoreManager). In this way we could get rid of the updating flag and its logic.

          I've attached a patch showing my fix

          Show
          bitstorm Andrea Del Bene added a comment - - edited Hi, I've noticed something interesting in method Session#destroy that might be part of the problem. As its JavaDoc states the method was meant to dispose session store and page manager when session is invalidated. At the moment there's no code for page manager, it was removed in (massive) commit 4977f66. I would expect that when this method is invoked it calls IPageManager#clear (implemented by PageStoreManager). In this way we could get rid of the updating flag and its logic. I've attached a patch showing my fix
          Hide
          svenmeier Sven Meier added a comment -

          In Tomcat re-setting the same value does not trigger valueUnbound() nor valueBound(), thus nobody resets the "updating" flag.

          When the session expires later on, valueUnbound() has no effect, because the "updating" flag is still set.

          Show
          svenmeier Sven Meier added a comment - In Tomcat re-setting the same value does not trigger valueUnbound() nor valueBound(), thus nobody resets the "updating" flag. When the session expires later on, valueUnbound() has no effect, because the "updating" flag is still set.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user svenmeier opened a pull request:

          https://github.com/apache/wicket/pull/233

          WICKET-6465 prevent unbound during storeTouchedPages only

          Why so complicated? We just want to prevent valueUnbound() from doing any harm while we're resetting the attribute during storeTouchedPages.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/svenmeier/wicket master

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/wicket/pull/233.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #233


          commit 8c7497144311923f7761e26eadedadfc9046f48e
          Author: Sven Meier <svenmeier@apache.org>
          Date: 2017-09-10T08:36:00Z

          WICKET-6465 prevent unbound during storeTouchedPages only


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user svenmeier opened a pull request: https://github.com/apache/wicket/pull/233 WICKET-6465 prevent unbound during storeTouchedPages only Why so complicated? We just want to prevent valueUnbound() from doing any harm while we're resetting the attribute during storeTouchedPages. You can merge this pull request into a Git repository by running: $ git pull https://github.com/svenmeier/wicket master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/wicket/pull/233.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #233 commit 8c7497144311923f7761e26eadedadfc9046f48e Author: Sven Meier <svenmeier@apache.org> Date: 2017-09-10T08:36:00Z WICKET-6465 prevent unbound during storeTouchedPages only
          Hide
          frantam Franta Mejta added a comment -

          The description is copied from WICKET-6457, more or less. This commit prevents the IPageStore.unbind() method to be called on Tomcat.

          Show
          frantam Franta Mejta added a comment - The description is copied from WICKET-6457 , more or less. This commit prevents the IPageStore.unbind() method to be called on Tomcat.
          Hide
          svenmeier Sven Meier added a comment -

          I don't see how WICKET-6387 is related. What do you mean by 'cleared' and 'logout'?

          Show
          svenmeier Sven Meier added a comment - I don't see how WICKET-6387 is related. What do you mean by 'cleared' and 'logout'?
          Hide
          bitstorm Andrea Del Bene added a comment -

          ok, I've reopened it.

          Show
          bitstorm Andrea Del Bene added a comment - ok, I've reopened it.
          Hide
          frantam Franta Mejta added a comment -

          It has been solved for a different servlet container.

          Show
          frantam Franta Mejta added a comment - It has been solved for a different servlet container.
          Hide
          bitstorm Andrea Del Bene added a comment -

          Hi,

          Isn't this a clone of WICKET-6457? If so the issue has been solved.

          Show
          bitstorm Andrea Del Bene added a comment - Hi, Isn't this a clone of WICKET-6457 ? If so the issue has been solved.
          Hide
          frantam Franta Mejta added a comment -

          Wait, what? I can't unset the Fix Version/s - this is not fixed at all.

          Show
          frantam Franta Mejta added a comment - Wait, what? I can't unset the Fix Version/s - this is not fixed at all.

            People

            • Assignee:
              svenmeier Sven Meier
              Reporter:
              frantam Franta Mejta
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development