-
15. Re: ConcurrentModificationException during session serializa
brian.stansberry Jun 25, 2009 12:20 PM (in response to stani0)Scott, this gets trickier in AS 5. In AS 5 the serialization of the attributes is deferred until JBoss Cache needs to serialize for replication. Which happens in ClusteredSessionValve.handleRequest:
[code
]finally
{
if (startedBatch)
{
tm.endBatch();
}
} -
16. Re: ConcurrentModificationException during session serializa
smarlow Jun 25, 2009 3:13 PM (in response to stani0)Brian,
We will have to try to get access to the StandardSessionFacade from there as well (perhaps through the ClusteredSession).
The concern I have is StandardSession.facade is not thread safe, and what's returned from getSession() needs to be if we are going to use it as a mutex.
We will only synchronize on the StandardSession.facade object and not modify it. Since we are not modifying the StandardSession.facade object, we do not need it to be thread safe. At least that is my thinking, let me know if I'm missing something :-)
There's no ref to facade outside StandardSession.getSession(), so ClusteredSession can override the getSession() method and replace the field with a volatile field and synchronize the construction if null.
I don't really understand the case when getSession would return null. I wonder if the null session case signify something special that needs to be preserved? For what its worth, the javadoc for Requests.getsession() is:/** * Return the session associated with this Request, creating one if necessary. */
-
17. Re: ConcurrentModificationException during session serializa
brian.stansberry Jun 25, 2009 4:30 PM (in response to stani0)"smarlow@redhat.com" wrote:
Brian,
We will have to try to get access to the StandardSessionFacade from there as well (perhaps through the ClusteredSession).
Yes. The tricky bit is if there are multiple sessions being replicated (as part of a cross-context request.) See below for more."smarlow@redhat.com" wrote:
The concern I have is StandardSession.facade is not thread safe, and what's returned from getSession() needs to be if we are going to use it as a mutex.
We will only synchronize on the StandardSession.facade object and not modify it. Since we are not modifying the StandardSession.facade object, we do not need it to be thread safe. At least that is my thinking, let me know if I'm missing something :-)
Sorry, I wasn't clear. The concern I have is concurrent requests to getSession() resulting in 2 facades being created. The classic singleton "getInstance() method" concurrency problem, except here it's not a static field. If you're going to lock on the facade you need to make sure there is only one facade. The current getSession() method doesn't do that.
There's no ref to facade outside StandardSession.getSession(), so ClusteredSession can override the getSession() method and replace the field with a volatile field and synchronize the construction if null.
What I described above is probably way overkill. Perhaps just calling getSession() in the constructor to ensure the field is initialized will work.
Seems my last post was misformatted and got cut off. Further to the AS 5 discussion, in the future it's possible the serialization will happen on a different thread. (Infinispan was looking at putting replication messages in a queue and then having seperate threads process them, including serialization.) If that were the case, using a synchronized block around the ClusteredSessionValve's tm.endBatch() call won't work.
But, the thing that actually gets serialized is an org.jboss.ha.framework.server.SimpleCachableMarshalledValue. A possible solution is to change that class to optionally pass in a "MutexSource" to its constructor:public interface MutexSource { public abstract Object getMutex(); }
If a MutexSource is available, SimpleCachableMarshalledValue.serialize() would call that method and synchronize on the return value. ClusteredSession would implement MutexSource by returning the result of a getSession() call.
This would also deal with the cross-context request issue noted above. -
18. Re: ConcurrentModificationException during session serializa
smarlow Jun 25, 2009 10:46 PM (in response to stani0)This isn't perfect but we might be able to convert the HTTP session attribute value to byte array before multiplexing and the reverse on the receiving end. I'm not thrilled about double buffering the attribute value (keeping a copy in serialized form and original object form), but maybe we are doing that already on the multiplexing side (in which case there isn't extra memory overhead).
The advantage of the above approach is that we could easily synchronize the relevant session facade during serialization/de-serialization. I'm not sure how that impacts the current design. What do you think? -
19. Re: ConcurrentModificationException during session serializa
smarlow Jun 26, 2009 6:46 AM (in response to stani0)After sleeping on it, I don't like the memory impact of my suggestion (serializing attribute values earlier). Your idea sounds better
If a MutexSource is available, SimpleCachableMarshalledValue.serialize() would call that method and synchronize on the return value. ClusteredSession would implement MutexSource by returning the result of a getSession() call.
This would also deal with the cross-context request issue noted above.
+1 -
20. Re: ConcurrentModificationException during session serializa
brian.stansberry Jun 26, 2009 11:14 AM (in response to stani0)Good. :-) I very deliberately eliminated the double buffering because of the memory cost.
FYI, before this thread got going I'd been thinking (vaguely) in terms of using read/write locks in AS 6 to better handle coordination. E.g., in ClusteredSessionValve acquire a RL before passing the request down the pipeline, release when it returns, acquire a WL before handling replication. Basically, allow concurrent access to a session but lock out requests while a thread is replicating.
The same thing could be done by having ClusteredSessionValve synchronize on the facade while passing the request down the pipeline, but then only one request can proceed at a time. -
21. Re: ConcurrentModificationException during session serializa
smarlow Jun 26, 2009 11:35 AM (in response to stani0)FYI, before this thread got going I'd been thinking (vaguely) in terms of using read/write locks in AS 6 to better handle coordination. E.g., in ClusteredSessionValve acquire a RL before passing the request down the pipeline, release when it returns, acquire a WL before handling replication. Basically, allow concurrent access to a session but lock out requests while a thread is replicating.
In the current case, the application code is synchronizing on the session facade object, so they would be left out if we synchronize on an internal r/w lock.
I wonder if we could somehow expose our (HTTP session level) r/w lock object to the application (in a reasonably portable way), so that they could also get a RL when getting the attribute values. Applications will also want to get a WL when setting attribute values. -
22. Re: ConcurrentModificationException during session serializa
brian.stansberry Jun 26, 2009 12:42 PM (in response to stani0)If we use a R/W lock, there shouldn't be any application activity during the periods where we are serializing etc. The WL we acquire prevents it.
-
23. Re: ConcurrentModificationException during session serializa
smarlow Jun 26, 2009 12:49 PM (in response to stani0)If we are holding the WL lock on some thread, couldn't another thread (processing an application request against the same HTTP session) access the locked attribute (reading/writing attribute values).
Maybe I missed how we would force application code to use our RW lock. Does that come for free with your approach? :-) -
24. Re: ConcurrentModificationException during session serializa
brian.stansberry Jun 26, 2009 2:09 PM (in response to stani0)The key is acquiring a RL in ClusteredSessionValve:
// let the servlet invocation go through Lock lock = manager.getReadLock(requestedSessionId); lock.lock(); try { if (isEvent) { getNext().event(request, response, event); } else { getNext().invoke(request, response); } } finally { lock.unlock(); }
The stuff inside that try/finally block encapsulates the application code's interaction with the session. The lock returned by manager.getReadlLock(...) is the RL from a java.util.concurrent.lock.ReadWriteLock. When any replication activity happens, it happens outside the above block and the corresponding WL is acquired.
Result is, application code will not be manipulating the session when the WL is held. Application code doesn't need access to the RL as it will never be conflicting with the replication code.
Unspoken and unproven assumption here: there is a sufficient benefit to having *guaranteed* lockout of application activity. One benefit is it removes the need for the application to synchronize. -
25. Re: ConcurrentModificationException during session serializa
smarlow Jun 26, 2009 2:45 PM (in response to stani0)I see, so we are obtaining the RL for the application (held while the application request is running). This avoids the cooperative locking scheme that would of required the application to lock on the sessionFacade object.
Some potential performance issues that could challenge this approach.
1. During http session replication, further http requests (from users that are involved with the replication) will block (at least while the session level write lock is held for the replication).
2. When a http request is completing that has modified http session attributes, the replication will be delayed until the write lock can be obtained. This could introduce unexpected performance delays.
3. Are there additional performance implications for synchronous replication? If the user waits in the browser for the replication to complete, they could be waiting a long time, depending on when the write lock can be obtained. -
26. Re: ConcurrentModificationException during session serializa
brian.stansberry Jun 26, 2009 5:44 PM (in response to stani0)To answer your #1 and #3, the key thing is to hold the WL for the minimum required time. You only have to hold it long enough to:
a) Accurately determine if anything needs to be replicated
b) Copy that into the DTO that's passed to the replication layer.
c) Serialize attributes.
Ideally a) and b) can be combined into one very quick step, although that would take some rework of logic. Then the WL could be acquired separately for c), which again should be quick. So you don't want to hold the WL for the duration of the replication.
Hmm, although that somewhat defeats the point, which is to get a consistent snapshot of the session. Separately locking for the serialization allows a request thread to sneak in and change the state of an attribute value.
#2 is more of an issue. By relying on synchronizing on the facade, we're basically giving priority to getting the replication out over consistency, and then letting the app enforce consistency by synchronizing on the facade. -
27. Re: ConcurrentModificationException during session serializa
smarlow Jul 1, 2009 10:36 AM (in response to stani0)Currently, deployed applications do not hold a RL lock while processing requests. If we introduce a RL lock to be held while processing requests, will that be slower?
There is some potential additional overhead to the ReadWriteLock (http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/ReadWriteLock.html).
Perhaps it would be better (performance wise) to synchronize on the session level facade object as I think the performance should be close to what applications currently see (whether or not they also synchronize on the session object when accessing mutable attributes).
What do you think? -
28. Re: ConcurrentModificationException during session serializa
brian.stansberry Jul 2, 2009 12:30 PM (in response to stani0)I (or you or nbelaevski if either is up for it) need to review the session replication code to see how much of an issue this is. I haven't had cycles to do that, which is why my comments here are vague.
If the issue is only protecting the attributes, then synching on the facade in the places where we serialize it is fine.
If something broader is needed, the best way to decide what to do is the perf test the two approaches.