You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@struts.apache.org by "S. Veyrié (JIRA)" <ji...@apache.org> on 2011/02/17 12:49:24 UTC
[jira] Created: (WW-3576) SessionMap is not thread-safe
SessionMap is not thread-safe
-----------------------------
Key: WW-3576
URL: https://issues.apache.org/jira/browse/WW-3576
Project: Struts 2
Issue Type: Bug
Components: Other
Affects Versions: 2.2.1
Reporter: S. Veyrié
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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Issue Comment Edited: (WW-3576) SessionMap is not
thread-safe
Posted by "S. Veyrié (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12995793#comment-12995793 ]
S. Veyrié edited comment on WW-3576 at 2/17/11 1:09 PM:
--------------------------------------------------------
DCL does not work before Java 5, but does work with "volatile" keyword on Java5+: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
Nevertheless, I agree this is only a quick-and-dirty fix. The best fix would be using Java 5 java.util.concurrent (or have synchronized(session) on the whole method, but there is a performance issue).
was (Author: sveyrie):
DCL does not work before Java 5, but works with "volatile" keyword on Java5+: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
Nevertherless, I agree this is only a quick-and-dirty fix. The best fix would be using Java 5 java.util.concurrent (or have synchronized(session) on the whole method, but there is a performance issue).
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: S. Veyrié
>
> 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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (WW-3576) SessionMap is not thread-safe
Posted by "Dave Newton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12995805#comment-12995805 ]
Dave Newton commented on WW-3576:
---------------------------------
Ah, okay; forgot about that (which is scary enough).
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: S. Veyrié
>
> 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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Issue Comment Edited: (WW-3576) SessionMap is not
thread-safe
Posted by "S. Veyrié (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12995793#comment-12995793 ]
S. Veyrié edited comment on WW-3576 at 2/17/11 12:52 PM:
---------------------------------------------------------
DCL does not work before Java 5, but works with "volatile" keyword on Java5+: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
Nevertherless, I agree this is only a quick-and-dirty fix. The best fix would be using Java 5 java.util.concurrent (or have synchronized(session) on the whole method, but there is a performance issue).
was (Author: sveyrie):
DCL does not work before Java 5, but works with "volatile" keyword on Java5+: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
Nevertherless, I agree this is only a quick-and-dirty fix. The best fix would be using Java 5 java.util.concurrent (or have synchronized on the while method, but there is the performance question).
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: S. Veyrié
>
> 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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (WW-3576) SessionMap is not thread-safe
Posted by "Lukasz Lenart (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Lukasz Lenart updated WW-3576:
------------------------------
Fix Version/s: 3.x
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: Sylvain Veyrié
> Assignee: Lukasz Lenart
> Fix For: 3.x
>
>
> 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.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Issue Comment Edited: (WW-3576) SessionMap is not
thread-safe
Posted by "Dave Newton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12995774#comment-12995774 ]
Dave Newton edited comment on WW-3576 at 2/17/11 11:59 AM:
-----------------------------------------------------------
DCL doesn't work and is JVM-implementation-dependent.
http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double.html
was (Author: newton_dave):
DCL doesn't work.
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: S. Veyrié
>
> 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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (WW-3576) SessionMap is not thread-safe
Posted by "Sylvain Veyrié (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12996430#comment-12996430 ]
Sylvain Veyrié commented on WW-3576:
------------------------------------
After some thinking about a quick patch, there are some other issues and questions.
Can SessionMap be extended? The class is non-final and its properties (including "session") are "protected". The only one example I found is a mock object in SessionMapTestCase.
If we want SessionMap to be extendable, we should care about how to share the lock, and that's not easy - see Brian Goets, /Java Concurrency in practice/ §3.5.
Another thing: currently, there are locks on "session" and "this" when "session" is to be initialized, but both are guarding "session".
I think the best should be to have an unique and specific private lock object, and set all properties to private.
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: Sylvain Veyrié
>
> 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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (WW-3576) SessionMap is not thread-safe
Posted by "Rene Gielen (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13166872#comment-13166872 ]
Rene Gielen commented on WW-3576:
---------------------------------
Since we want to ditch JDK 1.4 support for next major S2 version, the best solution to me seems to fix it then with the java.util.concurrent toolset at hand.
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: Sylvain Veyrié
> Assignee: Lukasz Lenart
> Fix For: 2.5
>
>
> 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.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Assigned: (WW-3576) SessionMap is not thread-safe
Posted by "Lukasz Lenart (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Lukasz Lenart reassigned WW-3576:
---------------------------------
Assignee: Lukasz Lenart
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: Sylvain Veyrié
> Assignee: Lukasz Lenart
>
> 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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (WW-3576) SessionMap is not thread-safe
Posted by "Dave Newton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12995774#comment-12995774 ]
Dave Newton commented on WW-3576:
---------------------------------
DCL doesn't work.
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: S. Veyrié
>
> 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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (WW-3576) SessionMap is not thread-safe
Posted by "S. Veyrié (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
S. Veyrié updated WW-3576:
--------------------------
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.
was:
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.
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: S. Veyrié
>
> 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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (WW-3576) SessionMap is not thread-safe
Posted by "S. Veyrié (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3576?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12995793#comment-12995793 ]
S. Veyrié commented on WW-3576:
-------------------------------
DCL does not work before Java 5, but works with "volatile" keyword on Java5+: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
Nevertherless, I agree this is only a quick-and-dirty fix. The best fix would be using Java 5 java.util.concurrent (or have synchronized on the while method, but there is the performance question).
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: S. Veyrié
>
> 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.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira