Uploaded image for project: 'Ignite'
  1. Ignite
  2. IGNITE-2710

Session not unbind from current request after invoking request.getSession().invalidate()

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 1.5.0.final
    • 1.6
    • cache
    • java8, tomcat8

    • Important

    Description

      System.out.println(request.getSession().getId());
      request.getSession().invalidate();
      System.out.println(request.getSession().getId());

      getSession() although return the same session after the invalidation.

      Attachments

        Activity

          shroman Roman Shtykh added a comment -

          A quick test invalidates session for me.
          Can you please provide simple configurations you use to reproduce it?

          shroman Roman Shtykh added a comment - A quick test invalidates session for me. Can you please provide simple configurations you use to reproduce it?
          crnlmcn YuxuanWang added a comment -

          ok,thanks
          ignite.xml:
          <bean id="session.cfg" class="org.apache.ignite.configuration.IgniteConfiguration">
          <property name="gridName" value="sessionGrid" />
          <property name="metricsLogFrequency" value="0" />
          <property name="gridLogger" >
          <bean class="org.apache.ignite.logger.slf4j.Slf4jLogger" />
          </property>
          <property name="cacheConfiguration">
          <bean class="org.apache.ignite.configuration.CacheConfiguration">
          <!-- Cache mode. -->
          <property name="name" value="webSession" />
          <property name="cacheMode" value="PARTITIONED" />
          <property name="backups" value="1" />
          </bean>
          </property>
          <property name="discoverySpi">
          <bean class="org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi">
          <property name="ipFinder">
          <bean class="org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder">
          <property name="addresses">
          <list>
          <value>localhost</value>
          </list>
          </property>
          </bean>
          </property>
          </bean>
          </property>
          </bean>

          web.xml:
          <listener>
          <listener-class>org.apache.ignite.startup.servlet.ServletContextListenerStartup</listener-class>
          </listener>
          <filter>
          <filter-name>IgniteWebSessionsFilter</filter-name>
          <filter-class>org.apache.ignite.cache.websession.WebSessionFilter</filter-class>
          </filter>
          <filter-mapping>
          <filter-name>IgniteWebSessionsFilter</filter-name>
          <url-pattern>/*</url-pattern>
          </filter-mapping>
          <context-param>
          <param-name>IgniteConfigurationFilePath</param-name>
          <param-value>/var/www/ignite/ignite-session.xml</param-value>
          </context-param>
          <context-param>
          <param-name>IgniteWebSessionsGridName</param-name>
          <param-value>sessionGrid</param-value>
          </context-param>
          <context-param>
          <param-name>IgniteWebSessionsCacheName</param-name>
          <param-value>webSession</param-value>
          </context-param>

          crnlmcn YuxuanWang added a comment - ok,thanks ignite.xml: <bean id="session.cfg" class="org.apache.ignite.configuration.IgniteConfiguration"> <property name="gridName" value="sessionGrid" /> <property name="metricsLogFrequency" value="0" /> <property name="gridLogger" > <bean class="org.apache.ignite.logger.slf4j.Slf4jLogger" /> </property> <property name="cacheConfiguration"> <bean class="org.apache.ignite.configuration.CacheConfiguration"> <!-- Cache mode. --> <property name="name" value="webSession" /> <property name="cacheMode" value="PARTITIONED" /> <property name="backups" value="1" /> </bean> </property> <property name="discoverySpi"> <bean class="org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi"> <property name="ipFinder"> <bean class="org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder"> <property name="addresses"> <list> <value>localhost</value> </list> </property> </bean> </property> </bean> </property> </bean> web.xml: <listener> <listener-class>org.apache.ignite.startup.servlet.ServletContextListenerStartup</listener-class> </listener> <filter> <filter-name>IgniteWebSessionsFilter</filter-name> <filter-class>org.apache.ignite.cache.websession.WebSessionFilter</filter-class> </filter> <filter-mapping> <filter-name>IgniteWebSessionsFilter</filter-name> <url-pattern>/*</url-pattern> </filter-mapping> <context-param> <param-name>IgniteConfigurationFilePath</param-name> <param-value>/var/www/ignite/ignite-session.xml</param-value> </context-param> <context-param> <param-name>IgniteWebSessionsGridName</param-name> <param-value>sessionGrid</param-value> </context-param> <context-param> <param-name>IgniteWebSessionsCacheName</param-name> <param-value>webSession</param-value> </context-param>
          shroman Roman Shtykh added a comment - - edited
          System.out.println(request.getSession().getId());
          request.getSession().invalidate();
          System.out.println(request.getSession().getId());
          

          Attributes are correctly nullified, but after the current session is invalidated, a new one with a different id has to be created (and that does not happen).

          Another problem is a bunch of methods should throw IllegalStateException on invalidaded session (http://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpSession.html)

          shroman Roman Shtykh added a comment - - edited System .out.println(request.getSession().getId()); request.getSession().invalidate(); System .out.println(request.getSession().getId()); Attributes are correctly nullified, but after the current session is invalidated, a new one with a different id has to be created (and that does not happen). Another problem is a bunch of methods should throw IllegalStateException on invalidaded session ( http://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpSession.html )
          shroman Roman Shtykh added a comment -

          The fix invalidates a session within a request, and enables the following scenario

          // returns a valid session
          request.getSession();
          
          // invalidates the session
          request.getSession().invalidate();
          
          // now the session is null
          request.getSession(false);
          
          // creates a new session
          request.getSession()
          

          https://github.com/apache/ignite/pull/530

          shroman Roman Shtykh added a comment - The fix invalidates a session within a request, and enables the following scenario // returns a valid session request.getSession(); // invalidates the session request.getSession().invalidate(); // now the session is null request.getSession( false ); // creates a new session request.getSession() https://github.com/apache/ignite/pull/530
          shroman Roman Shtykh added a comment - - edited

          vkulichenko Can I ask you to review it and mute IgniteWebSessionSelfTestSuite$WebSessionTransactionalSelfTest.testInvalidatedSession (or give me permissions)?

          shroman Roman Shtykh added a comment - - edited vkulichenko Can I ask you to review it and mute IgniteWebSessionSelfTestSuite$WebSessionTransactionalSelfTest.testInvalidatedSession (or give me permissions)?
          shroman Roman Shtykh added a comment -

          sboikov Probably you are the right person to have a look?

          shroman Roman Shtykh added a comment - sboikov Probably you are the right person to have a look?

          Roman,

          Sorry for the delay. I will take a look at this today or tomorrow.

          vkulichenko Valentin Kulichenko added a comment - Roman, Sorry for the delay. I will take a look at this today or tomorrow.
          shroman Roman Shtykh added a comment -

          Valentin, thank you!

          shroman Roman Shtykh added a comment - Valentin, thank you!

          Roman,

          Looks good,
          but:
          1) Is there any reason to store isValid at writeExternal? I see that cache.remove(ses) happens at ses.invalidate().
          I removed isValid storage and see no test failures.
          2) Seems that "GenuineSession" should be invalidated at ses.invalidate, not at next getSession() request. (super.invalidate() should be invoked)
          3) WebSession ses at RequestWrapper should be marked as volatile
          4) getSession at RequestWrapper should return invalid session instead of null

          avinogradov Anton Vinogradov (Obsolete, actual is "av") added a comment - Roman, Looks good, but: 1) Is there any reason to store isValid at writeExternal? I see that cache.remove(ses) happens at ses.invalidate(). I removed isValid storage and see no test failures. 2) Seems that "GenuineSession" should be invalidated at ses.invalidate, not at next getSession() request. (super.invalidate() should be invoked) 3) WebSession ses at RequestWrapper should be marked as volatile 4) getSession at RequestWrapper should return invalid session instead of null
          shroman Roman Shtykh added a comment -

          Anton,

          Thanks for your comments! I have made the changes.

          1) -> You are absolutely right. Externalizing isValid is not needed here – by the current logic the session is completely deleted from the cache, and when it is not found a new session is created. I think I will need to make isValid transient then.

          2) -> HttpSession is an interface. I could not find a cleaner way, so I invalidate the gunuine session on every subsequent getSession() if it is invalid.

          3) -> Good point, thank you!

          4) -> I was thinking of it too. But doing some investigations and looking at users' code at github, it looks like many users expects null. Tomcat produces null too. Just want to be sure users won't be surprised switching to Ignite. What do you think?

          shroman Roman Shtykh added a comment - Anton, Thanks for your comments! I have made the changes. 1) -> You are absolutely right. Externalizing isValid is not needed here – by the current logic the session is completely deleted from the cache, and when it is not found a new session is created. I think I will need to make isValid transient then. 2) -> HttpSession is an interface. I could not find a cleaner way, so I invalidate the gunuine session on every subsequent getSession() if it is invalid. 3) -> Good point, thank you! 4) -> I was thinking of it too. But doing some investigations and looking at users' code at github, it looks like many users expects null . Tomcat produces null too. Just want to be sure users won't be surprised switching to Ignite. What do you think?

          Roman,

          2) I see that gunuine session is a Websession constructor param, possible we have to store it as a field and invalidate at ses.invaliate()? Or we can invalidate it somehow using sesId? What do you think?
          4) Sounds good in this case.

          avinogradov Anton Vinogradov (Obsolete, actual is "av") added a comment - Roman, 2) I see that gunuine session is a Websession constructor param, possible we have to store it as a field and invalidate at ses.invaliate()? Or we can invalidate it somehow using sesId? What do you think? 4) Sounds good in this case.
          shroman Roman Shtykh added a comment - - edited

          Anton,

          I changed the implementation by storing a genuine session, as you advised. Looks nice and simple to me.
          isValid's use becomes trivial – just to follow specifications and produce IllegalStateException when invalid.

          shroman Roman Shtykh added a comment - - edited Anton, I changed the implementation by storing a genuine session, as you advised. Looks nice and simple to me. isValid 's use becomes trivial – just to follow specifications and produce IllegalStateException when invalid.

          Roman,

          1) I think that "if (cache.get(ses.getId()) == null) {" should be replaced by isValid() check because cache.get if much more heavy than reading volatile.
          2) Also new session should be created on demand at any cases, not only if current invalidated? Possible previous one should be invalidated in case new one demanded.

          avinogradov Anton Vinogradov (Obsolete, actual is "av") added a comment - Roman, 1) I think that "if (cache.get(ses.getId()) == null) {" should be replaced by isValid() check because cache.get if much more heavy than reading volatile. 2) Also new session should be created on demand at any cases, not only if current invalidated? Possible previous one should be invalidated in case new one demanded.
          shroman Roman Shtykh added a comment -

          Anton,

          1) Yes, you are right. I have reverted this change.
          2) According to the specs, it is created only when there is no session and create is true. http://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServletRequest.html#getSession-boolean-

          shroman Roman Shtykh added a comment - Anton, 1) Yes, you are right. I have reverted this change. 2) According to the specs, it is created only when there is no session and create is true. http://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServletRequest.html#getSession-boolean-

          Roman,

          Now looks good to me.

          Please change genuineSession to ses according to https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules and check TeamCity.
          P.s. I'm not an expert in servlets, so in case you're not sure at some changes lets ask someone to review result of our discussions

          avinogradov Anton Vinogradov (Obsolete, actual is "av") added a comment - Roman, Now looks good to me. Please change genuineSession to ses according to https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules and check TeamCity. P.s. I'm not an expert in servlets, so in case you're not sure at some changes lets ask someone to review result of our discussions
          shroman Roman Shtykh added a comment -

          Thank you! Maybe Valentin has some comments?

          It seems session replication has more issues. I will open other tickets if I can confirm.

          Also,IgniteWebSessionSelfTestSuite$WebSessionTransactionalSelfTest.testInvalidatedSession will have to be muted. Whom should I ask?

          shroman Roman Shtykh added a comment - Thank you! Maybe Valentin has some comments? It seems session replication has more issues. I will open other tickets if I can confirm. Also,IgniteWebSessionSelfTestSuite$WebSessionTransactionalSelfTest.testInvalidatedSession will have to be muted. Whom should I ask?

          Roman,

          it's not a good case to mute untill it contains fail("https://issues.apache.org/jira/browse/IGNITE-*");
          Someone, for example me, will mute that when see such failure.
          P.s. ask Val about review by email.

          avinogradov Anton Vinogradov (Obsolete, actual is "av") added a comment - Roman, it's not a good case to mute untill it contains fail("https://issues.apache.org/jira/browse/IGNITE-*"); Someone, for example me, will mute that when see such failure. P.s. ask Val about review by email.
          shroman Roman Shtykh added a comment -

          Anton,

          Got it. It contains fail(...) for transactional mode related to a jira issue. Let's see if Val has any comments, and someone can mute it after I merge.

          shroman Roman Shtykh added a comment - Anton, Got it. It contains fail(...) for transactional mode related to a jira issue. Let's see if Val has any comments, and someone can mute it after I merge.

          Changes make sense to me. Couple of questions though:

          • Why not all methods are guarded by if (!isValid) check? Is it on purpose?
          • Why the test doesn't work in transactional cache?
          vkulichenko Valentin Kulichenko added a comment - Changes make sense to me. Couple of questions though: Why not all methods are guarded by if (!isValid) check? Is it on purpose? Why the test doesn't work in transactional cache?
          shroman Roman Shtykh added a comment -

          Thank you!
          1) methods are guarded according to the HttpSession specs. Probably the only left are s/getMaxInactiveInterval() and getId() but specs don't say how it should behave when the session is invalid, it says they must return a value. All the rest are deprecated and call guarded methods.
          http://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpSession.html
          2) I guess it's the same IGNITE-810 which doesn't work for testRestarts()

          shroman Roman Shtykh added a comment - Thank you! 1) methods are guarded according to the HttpSession specs. Probably the only left are s/getMaxInactiveInterval() and getId() but specs don't say how it should behave when the session is invalid, it says they must return a value. All the rest are deprecated and call guarded methods. http://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpSession.html 2) I guess it's the same IGNITE-810 which doesn't work for testRestarts()

          OK, I think it can be merged then.

          vkulichenko Valentin Kulichenko added a comment - OK, I think it can be merged then.
          shroman Roman Shtykh added a comment -

          Merged. Thank you, guys for the reviews!

          shroman Roman Shtykh added a comment - Merged. Thank you, guys for the reviews!

          People

            shroman Roman Shtykh
            crnlmcn YuxuanWang
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: