Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.2.1
    • Fix Version/s: 3.0
    • Component/s: Other
    • Labels:
      None

      Description

      Searching for a bug after some stress test (Exception on "getAttribute": already invalidated session), I reviewed SessionMap.java.

      Following WW-239, SessionMap has been made thread-safe.

      All methods are like this :

      public void doSomething() {
      if (session == null)

      { return; }

      synchronized (session)

      { session.doSometing(); }

      }

      For example:
      public void invalidate() {
      if (session == null)

      { return; }

      synchronized (session)

      { session.invalidate(); session = null; entries = null; }

      }

      IMHO this is not thread-safe. With the example of invalidate(), if there is a context switch just before the synchronized, the nullity is no more checked. If another invalidate() is called at least a NPE can be thrown. There are probably other side-effects like my exception problem.

      As now Struts 2 only supports Java 5+, there is two ways to fix it :

      • use a double-check-locking (DCL) and set session "volatile" (easy way)
      • use java.util.concurrent instead of synchronized keyword

      If you agree and choose one of those fixes, I can provide a patch.

      For the moment, I don't know if my bug is resolved if we directly use javax session, without this wrapper.

        Activity

        Lukasz Lenart made changes -
        Fix Version/s 3.x [ 12319158 ]
        Lukasz Lenart made changes -
        Assignee Lukasz Lenart [ lukaszlenart ]
        Sylvain Veyrié made changes -
        Field Original Value New Value
        Description Searching for a bug after some stress test (Exception on "getAttribute": already invalidated session), I reviewed SessionMap.java.

        Following WW-239, SessionMap has been made thread-safe.

        All methods are like this :

        public void doSomething() {
          if (session == null) {
        return;
          }
         
          synchronized (session) {
        session.doSometing();
          }
        }

        For example:
        public void invalidate() {
        if (session == null) {
        return;
        }

        synchronized (session) {
        session.invalidate();
        session = null;
        entries = null;
        }
        }

        IMHO this is not thread-safe. With the example of invalidate(), if there is a context switch just before the synchronized, the nullity is no more checked. If another invalidate() is called at least a NPE can be thrown. There are probably other side-effects like my exception problem).

        As now Struts 2 only support Java 5+, there is two ways to fix it :
        * use a double-check-locking (DCL) and set session "volatile" (easy way)
        * use java.util.concurrent instead of synchronized keyword

        If you agree and choose one of those fixes, I can provide a patch.

        For the moment, I don't know if my bug is resolved if we directly use javax session, without this wrapper.
        Searching for a bug after some stress test (Exception on "getAttribute": already invalidated session), I reviewed SessionMap.java.

        Following WW-239, SessionMap has been made thread-safe.

        All methods are like this :

        public void doSomething() {
          if (session == null) {
        return;
          }
         
          synchronized (session) {
        session.doSometing();
          }
        }

        For example:
        public void invalidate() {
        if (session == null) {
        return;
        }

        synchronized (session) {
        session.invalidate();
        session = null;
        entries = null;
        }
        }

        IMHO this is not thread-safe. With the example of invalidate(), if there is a context switch just before the synchronized, the nullity is no more checked. If another invalidate() is called at least a NPE can be thrown. There are probably other side-effects like my exception problem.

        As now Struts 2 only supports Java 5+, there is two ways to fix it :
        * use a double-check-locking (DCL) and set session "volatile" (easy way)
        * use java.util.concurrent instead of synchronized keyword

        If you agree and choose one of those fixes, I can provide a patch.

        For the moment, I don't know if my bug is resolved if we directly use javax session, without this wrapper.
        Sylvain Veyrié created issue -

          People

          • Assignee:
            Lukasz Lenart
            Reporter:
            Sylvain Veyrié
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development