subject:"\[IMP\] synchronization on session object in Cocoon"

It suddenly dawned on me that the code below is synchronizing on the 
session, not a local object. Duh! Still, this seems like a very, very 
bad idea to me, as I'm not sure how that would impact the servlet container.

public Session getSession(boolean create) {
   // we must assure a 1:1-mapping of server session to cocoon session
   javax.servlet.http.HttpSession serverSession = this.req.getSession(create);
   if (serverSession != null) {
   synchronized (serverSession) {
   // retrieve existing wrapper
   this.session =
(HttpSession)serverSession.getAttribute(HTTP_SESSION);
   if (this.session == null) {
   // create wrapper
   this.session = new HttpSession(serverSession);
   serverSession.setAttribute(HTTP_SESSION, this.session);
   }
   }
   } else {
   // invalidate
   this.session = null;
   }
   return this.session;
}

Re: [IMP] synchronization on session object in Cocoon

I ran through this thread and made some observations:
1. There can only be one Cocoon Session per HttpSession. Creation of 
this must be thread safe.
2. The code propsed will not work:

public Session getSession(boolean create) {
   // we must assure a 1:1-mapping of server session to cocoon session
   javax.servlet.http.HttpSession serverSession = this.req.getSession(create);
   if (serverSession != null) {
   synchronized (serverSession) {
   // retrieve existing wrapper
   this.session =
(HttpSession)serverSession.getAttribute(HTTP_SESSION);
   if (this.session == null) {
   // create wrapper
   this.session = new HttpSession(serverSession);
   serverSession.setAttribute(HTTP_SESSION, this.session);
   }
   }
   } else {
   // invalidate
   this.session = null;
   }
   return this.session;
}
serverSession is a local variable. Synchronizing it accomplishes nothing 
since every caller gets their own copy.  Sytnchronizing on a member of 
the HttpRequest object also accomplishes nothing.

I'm still trying to think of a solution I like.
Ralph

Re: [IMP] synchronization on session object in Cocoon

Ralph Goers Ralph.Goers at dslextreme.com writes:

 serverSession is a local variable. Synchronizing it accomplishes nothing 
 since every caller gets their own copy.  Sytnchronizing on a member of 
 the HttpRequest object also accomplishes nothing.

Huh? Why shall synchronization not work? It is not synched on the local variable
but on the object that is referenced by the local variable.

Joerg

Re: [IMP] synchronization on session object in Cocoon

Sylvain Wallez sylvain at apache.org writes:

 This approach has the problem of entering a synchronized block each time 
 getSession is called. Although this may not that be much a problem in 
 this particular case because it's unlikely that many parallel requests 
 exist for a single request, we may totally avoid it except once.

http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double_p.html

Joerg

Re: [IMP] synchronization on session object in Cocoon

Sylvain Wallez sylvain at apache.org writes:

 Or more simply we could store the wrapper in the session itself using an
 attribute. That way it would be guaranteed to be created only once.
 
 
 Yes, that's another possibility I also had in mind. But on the one hand
 this smells a bit (storing a wrapper in the object that the wrapper
 wraps), on the other hand you can not restrict access to it, so it can
 be manipulated from somewhere else. But the Map solution can indeed be
 very resource consuming and a bottle neck.
 
 I am having second thoughts about the proposed solution.  If the Cocoon
 session is stored as parasitic session attribute, the container can no
 longer serialize it for persistance or cluster replication.  Not that one
 really needs to save/restore this particular attribute but it will cause
 nasty log messages at the very least.

Besides the other mentioned points that's indeed a no-go for this trivial
implementation IMO.

 Good point. A solution can be to for the wrapper to implement 
 HttpSessionActivationListener and Serializable, and keep the session it 
 wraps as a transient attribute (to avoid its serialization), and restore 
 it on session activation (the HttpSessionEvent contains the session).

This solution looks really cool and elegant. Unfortunately
HttpSessionActivationListener is Servlet Spec 2.3. In Cocoon 2.1. we are still
on 2.2 and I guess (and suggest) we will stay with that. So only the WeakHashMap
solution remains.

 I think now that a private WeakHashMap (to leave session timeout to the
 container) should be the preferred solution.
 
 That's a solution that avoids the parasitic attribute. Where should this 
 Map be stored? As a static field of HttpSession?

Why not HttpRequest where it is needed because of getSession() methods? Of
course we could forward getSession() to a factory method in HttpSession that
decides whether to create a new wrapper or return the existing one.

I have an implementation with map in HttpRequest and without double-checked
locking idiom. Shall I commit it?

Joerg

Re: [IMP] synchronization on session object in Cocoon

2005-05-12 Thread Sylvain Wallez

Joerg Heinicke wrote:
Sylvain Wallez sylvain at apache.org writes:
 

Or more simply we could store the wrapper in the session itself using an
attribute. That way it would be guaranteed to be created only once.
  

   

Yes, that's another possibility I also had in mind. But on the one hand
this smells a bit (storing a wrapper in the object that the wrapper
wraps), on the other hand you can not restrict access to it, so it can
be manipulated from somewhere else. But the Map solution can indeed be
very resource consuming and a bottle neck.
 

I am having second thoughts about the proposed solution.  If the Cocoon
session is stored as parasitic session attribute, the container can no
longer serialize it for persistance or cluster replication.  Not that one
really needs to save/restore this particular attribute but it will cause
nasty log messages at the very least.
 

Besides the other mentioned points that's indeed a no-go for this trivial
implementation IMO.
 

Good point. A solution can be to for the wrapper to implement 
HttpSessionActivationListener and Serializable, and keep the session it 
wraps as a transient attribute (to avoid its serialization), and restore 
it on session activation (the HttpSessionEvent contains the session).
   

This solution looks really cool and elegant. Unfortunately
HttpSessionActivationListener is Servlet Spec 2.3. In Cocoon 2.1. we are still
on 2.2 and I guess (and suggest) we will stay with that. So only the WeakHashMap
solution remains.
 

2.2, really? It's at least 4 years old!
I think now that a private WeakHashMap (to leave session timeout to the
container) should be the preferred solution.
 

That's a solution that avoids the parasitic attribute. Where should this 
Map be stored? As a static field of HttpSession?
   

Why not HttpRequest where it is needed because of getSession() methods? Of
course we could forward getSession() to a factory method in HttpSession that
decides whether to create a new wrapper or return the existing one.
I have an implementation with map in HttpRequest and without double-checked
locking idiom. Shall I commit it?
 

Sure!
Sylvain
--
Sylvain WallezAnyware Technologies
http://apache.org/~sylvainhttp://anyware-tech.com
Apache Software Foundation Member Research  Technology Director

Re: [IMP] synchronization on session object in Cocoon

Sylvain Wallez sylvain at apache.org writes:

 This solution looks really cool and elegant. Unfortunately
 HttpSessionActivationListener is Servlet Spec 2.3. In Cocoon 2.1. we are
 still on 2.2 and I guess (and suggest) we will stay with that. So only the
 WeakHashMap solution remains.
   
 2.2, really? It's at least 4 years old!

Do you want to do such a step in a maintenance release?

Joerg

RE: Re: [IMP] synchronization on session object in Cocoon

2005-05-12 Thread Nathaniel Alfred

-Original Message-
From: news [mailto:[EMAIL PROTECTED] Behalf Of Joerg Heinicke
Sent: Donnerstag, 12. Mai 2005 11:16
To: dev@cocoon.apache.org
Subject: Re: [IMP] synchronization on session object in Cocoon

I have an implementation with map in HttpRequest and without 
double-checked
locking idiom. Shall I commit it?

Joerg

I think there is a memory leak in 
http://svn.apache.org/viewcvs?rev=169806view=rev.
There is a strong reference session.wrappedSession from value to key in

// create new wrapper
session = new HttpSession(serverSession);
sessions.put(serverSession, session);

which causes the WeakHashMap to keep the entries forever.

See the Implementation Note in 
http://java.sun.com/j2se/1.4.2/docs/api/java/util/WeakHashMap.html.

Cheers, Alfred.

This message is for the named person's use only. It may contain confidential, 
proprietary or legally privileged information. No confidentiality or privilege 
is waived or lost by any mistransmission. If you receive this message in error, 
please notify the sender urgently and then immediately delete the message and 
any copies of it from your system. Please also immediately destroy any 
hardcopies of the message. You must not, directly or indirectly, use, disclose, 
distribute, print, or copy any part of this message if you are not the intended 
recipient. The sender's company reserves the right to monitor all e-mail 
communications through their networks. Any views expressed in this message are 
those of the individual sender, except where the message states otherwise and 
the sender is authorised to state them to be the views of the sender's company.

Re: Re: [IMP] synchronization on session object in Cocoon

Nathaniel Alfred Alfred.Nathaniel at swx.com writes:

 I think there is a memory leak in
 http://svn.apache.org/viewcvs?rev=169806view=rev.
 There is a strong reference session.wrappedSession from value to key in
 
 // create new wrapper
 session = new HttpSession(serverSession);
 sessions.put(serverSession, session);
 
 which causes the WeakHashMap to keep the entries forever.
 
 See the Implementation Note in
 http://java.sun.com/j2se/1.4.2/docs/api/java/util/WeakHashMap.html.

Ah, thanks. I did not have any experience with the different types of references
- and though I read it before implementing I did not take care about exactly
that case. Now it's fixed (and I hope I did not add a typo or so as I have no
Java-understanding editor on this PC).

Joerg

Re: [IMP] synchronization on session object in Cocoon

In addition, I really don't like the implementation that was checked 
in.  Frankly, this is a case where I would look into leveraging 
backport-util-concurrent 
(http://www.mathcs.emory.edu/dcl/util/backport-util-concurrent/) if that 
code can be made to work in JDK 1.3.

What I would look to do is to add the wrapper to a Map using the session 
id as the key, and then add an attribute to the session that is an 
instance of a class that implements SessionBindingListener. Then when 
the sessionDestroyed method is called the wrapper can be removed from 
the Map.

Thus the Map would be the object being synchronized, such as
String id = serverSession.getId();
syncronized(map) {
   if (!map.contains(id)) {
   return (Session)map.get(id);
   }
   else {
   Session session = new HttpSession(serverSession);
   map.put(id, session);
   return session;
   }
}
Nathaniel Alfred wrote:
I have an implementation with map in HttpRequest and without 
double-checked
locking idiom. Shall I commit it?

Joerg
   

I think there is a memory leak in 
http://svn.apache.org/viewcvs?rev=169806view=rev.
There is a strong reference session.wrappedSession from value to key in
   // create new wrapper
   session = new HttpSession(serverSession);
   sessions.put(serverSession, session);
which causes the WeakHashMap to keep the entries forever.
See the Implementation Note in 
http://java.sun.com/j2se/1.4.2/docs/api/java/util/WeakHashMap.html.
Cheers, Alfred.

Re: [IMP] synchronization on session object in Cocoon

In addition, I really don't like the implementation that was checked 
in.  Frankly, this is a case where I would look into leveraging 
backport-util-concurrent 
(http://www.mathcs.emory.edu/dcl/util/backport-util-concurrent/) if that 
code can be made to work in JDK 1.3.

What I would look to do is to add the wrapper to a Map using the session 
id as the key, and then add an attribute to the session that is an 
instance of a class that implements SessionBindingListener. Then when 
the sessionDestroyed method is called the wrapper can be removed from 
the Map.

Thus the Map would be the object being synchronized, such as
String id = serverSession.getId();
syncronized(map) {
   if (!map.contains(id)) {
   return (Session)map.get(id);
   }
   else {
   Session session = new HttpSession(serverSession);
   map.put(id, session);
   return session;
   }
}
Nathaniel Alfred wrote:
I have an implementation with map in HttpRequest and without 
double-checked
locking idiom. Shall I commit it?

Joerg
   

I think there is a memory leak in 
http://svn.apache.org/viewcvs?rev=169806view=rev.
There is a strong reference session.wrappedSession from value to key in
   // create new wrapper
   session = new HttpSession(serverSession);
   sessions.put(serverSession, session);
which causes the WeakHashMap to keep the entries forever.
See the Implementation Note in 
http://java.sun.com/j2se/1.4.2/docs/api/java/util/WeakHashMap.html.
Cheers, Alfred.

RE: [IMP] synchronization on session object in Cocoon

-Original Message-
From: news [mailto:[EMAIL PROTECTED] Behalf Of Joerg Heinicke
Sent: Dienstag, 10. Mai 2005 18:56
To: dev@cocoon.apache.org
Subject: [IMP] synchronization on session object in Cocoon

As you can see on every request a new wrapper is instantiated
which is really
bad. It is not possible to synchronize on Cocoon session
objects. What we
probably need is a Map mapping the server sessions to the
wrapper objects.

WDYT?

Joerg

-1

Servlet API [1] says Session information is scoped only to the current web
application (ServletContext), so information stored in one context will not be
directly visible in another. I read that as A new session object must be
create for requests in different contexts and may be create for requests in the
same context.

So the circumstances under which synchronized(session) works is dependent on
the servlet implementation.

But the String pool comes to the rescue. This should work for all sessions
within the same JVM:

synchronized(session.getId().intern()) {
...
}

Cheers, Alfred.

[1]:
http://jakarta.apache.org/tomcat/tomcat-4.1-doc/servletapi/javax/servlet/http/HttpSession.html

This message is for the named person's use only. It may contain confidential,
proprietary or legally privileged information. No confidentiality or privilege
is waived or lost by any mistransmission. If you receive this message in error,
please notify the sender urgently and then immediately delete the message and
any copies of it from your system. Please also immediately destroy any
hardcopies of the message. You must not, directly or indirectly, use, disclose,
distribute, print, or copy any part of this message if you are not the intended
recipient. The sender's company reserves the right to monitor all e-mail
communications through their networks. Any views expressed in this message are
those of the individual sender, except where the message states otherwise and
the sender is authorised to state them to be the views of the sender's company.

Re: [IMP] synchronization on session object in Cocoon

2005-05-11 Thread Sylvain Wallez

Joerg Heinicke wrote:
Hello,
I think I have found a more general problem with synchronization in Cocoon. I
tried to solve my problem with synchronization in flow script with an
intermediate object. This object handles the synchronized instantiation of my
component. Unfortunately I found out that synchronized (session) { ... } still
did not work - two requests of the same session run into that block at the same
time. I did some remote debugging. First I thought the reason is in flow script
as it also wraps the session, but it unwraps it later. The reason is in
HttpRequest class. Have a look at the code [1]:
public Session getSession(boolean create) {
   javax.servlet.http.HttpSession serverSession = this.req.getSession(create);
   if ( null != serverSession) {
   if ( null != this.session ) {
   if ( this.session.wrappedSession != serverSession ) {
   // update wrapper
   this.session.wrappedSession = serverSession;
   }
   } else {
   // new wrapper
   this.session = new HttpSession( serverSession );
   }
   } else {
   // invalidate
   this.session = null;
   }
   return this.session;
}
As you can see on every request a new wrapper is instantiated which is really
bad. It is not possible to synchronize on Cocoon session objects.
Wow, very good point!
What we probably need is a Map mapping the server sessions to the wrapper objects.
 

Or more simply we could store the wrapper in the session itself using an 
attribute. That way it would be guaranteed to be created only once.

Sylvain
--
Sylvain WallezAnyware Technologies
http://apache.org/~sylvainhttp://anyware-tech.com
Apache Software Foundation Member Research  Technology Director

Re: [IMP] synchronization on session object in Cocoon

2005-05-11 Thread Joerg Heinicke

Nathaniel Alfred Alfred.Nathaniel at swx.com writes:

  As you can see on every request a new wrapper is instantiated
  which is really
  bad. It is not possible to synchronize on Cocoon session
  objects. What we
  probably need is a Map mapping the server sessions to the
  wrapper objects.

 Servlet API [1] says Session information is scoped only to the current
 web application (ServletContext), so information stored in one context will
 not be directly visible in another.  I read that as A new session object
 must be create for requests in different contexts and may be create for
 requests in the same context.

Though I see your point I don't know if I can follow your interpretation.

 So the circumstances under which synchronized(session) works is dependent
 on the servlet implementation.

I can not imagine that such an important point is not metioned in the
servlet
spec. I have already downloaded it, but did not had a look into it for this
point.

 But the String pool comes to the rescue.  This should work for all
 sessions within the same JVM:

synchronized(session.getId().intern()) {
   ...
}

That's not more than a work around IMO.

Furthermore your interpretation seems not to be common sense in Cocoon
project:

synchronized[ ]?\(session\) - 16 matches in project Cocoon 2.1.7

core:
org.apache.cocoon.Cocoon (1 match)

authentication-fw:
org.apache.cocoon.webapps.authentication.generation.ConfigurationGenerator
(1 match)

portal-fw:
org.apache.cocoon.webapps.portal.components.PortalManagerImpl (4 matches)

session-fw:
org.apache.cocoon.webapps.session.components.DefaultFormManager (2 matches)
org.apache.cocoon.webapps.session.components.DefaultContextManager (5
matches)
org.apache.cocoon.webapps.session.components.DefaultSessionManager (1 match)

xsp:
org.apache.cocoon.components.xscript.XScriptManagerImpl (1 match)
org.apache.cocoon.components.language.markup.xsp.XSPUtil (1 match)

Joerg

RE: Re: [IMP] synchronization on session object in Cocoon

 -Original Message-
 From: news [mailto:[EMAIL PROTECTED] Behalf Of Joerg Heinicke
 Sent: Mittwoch, 11. Mai 2005 13:50
 To: dev@cocoon.apache.org
 Subject: Re: [IMP] synchronization on session object in Cocoon
...
  But the String pool comes to the rescue.  This should work for all
  sessions within the same JVM:

 synchronized(session.getId().intern()) {
...
 }

 That's not more than a work around IMO.

 Furthermore your interpretation seems not to be common sense in Cocoon
 project:

 synchronized[ ]?\(session\) - 16 matches in project Cocoon 2.1.7

 core:
 org.apache.cocoon.Cocoon (1 match)

 authentication-fw:
 org.apache.cocoon.webapps.authentication.generation.Configurat
 ionGenerator
 (1 match)

 portal-fw:
 org.apache.cocoon.webapps.portal.components.PortalManagerImpl 
 (4 matches)

 session-fw:
 org.apache.cocoon.webapps.session.components.DefaultFormManage
 r (2 matches)
 org.apache.cocoon.webapps.session.components.DefaultContextManager (5
 matches)
 org.apache.cocoon.webapps.session.components.DefaultSessionMan
 ager (1 match)

 xsp:
 org.apache.cocoon.components.xscript.XScriptManagerImpl (1 match)
 org.apache.cocoon.components.language.markup.xsp.XSPUtil (1 match)

 Joerg

AFAIKS is the purpose of the existing synchronized(session) bits to
protect the consistency of the session object in case it is shared
between parallel executing requests.

IIUC you need a lock object to really synchronize parallel requests.  
There I'd say the session is the wrong candidate due to the mentioned
possible dependency on the servlet container.

I am just afraid that keeping the wrapped sessions in a private map
may introduce a memory leak.  But maybe WeakHashMap would take care
of this...

Cheers, Alfred.

This message is for the named person's use only. It may contain confidential, 
proprietary or legally privileged information. No confidentiality or privilege 
is waived or lost by any mistransmission. If you receive this message in error, 
please notify the sender urgently and then immediately delete the message and 
any copies of it from your system. Please also immediately destroy any 
hardcopies of the message. You must not, directly or indirectly, use, disclose, 
distribute, print, or copy any part of this message if you are not the intended 
recipient. The sender's company reserves the right to monitor all e-mail 
communications through their networks. Any views expressed in this message are 
those of the individual sender, except where the message states otherwise and 
the sender is authorised to state them to be the views of the sender's company.

Re: [IMP] synchronization on session object in Cocoon

2005-05-11 Thread Joerg Heinicke

Sylvain Wallez sylvain at apache.org writes:

 I think I have found a more general problem with synchronization in Cocoon. I
 tried to solve my problem with synchronization in flow script with an
 intermediate object. This object handles the synchronized instantiation of my
 component. Unfortunately I found out that synchronized (session) { ... }
 still did not work - two requests of the same session run into that block at
 the same time. I did some remote debugging. First I thought the reason is in
 flow script as it also wraps the session, but it unwraps it later. The reason
 is in HttpRequest class. Have a look at the code [1]:

 As you can see on every request a new wrapper is instantiated which is really
 bad. It is not possible to synchronize on Cocoon session objects.
 
 Wow, very good point!
 
 What we probably need is a Map mapping the server sessions to the wrapper
 objects.
   
 Or more simply we could store the wrapper in the session itself using an 
 attribute. That way it would be guaranteed to be created only once.

Yes, that's another possibility I also had in mind. But on the one hand this
smells a bit (storing a wrapper in the object that the wrapper wraps), on the
other hand you can not restrict access to it, so it can be manipulated from
somewhere else. But the Map solution can indeed be very resource consuming and a
bottle neck.

I have implemented the session attribute solution. Would be nice if you can
review it:

public Session getSession(boolean create) {
// we must assure a 1:1-mapping of server session to cocoon session
javax.servlet.http.HttpSession serverSession = this.req.getSession(create);
if (serverSession != null) {
synchronized (serverSession) {
// retrieve existing wrapper
this.session =
(HttpSession)serverSession.getAttribute(HTTP_SESSION);
if (this.session == null) {
// create wrapper
this.session = new HttpSession(serverSession);
serverSession.setAttribute(HTTP_SESSION, this.session);
}
}
} else {
// invalidate
this.session = null;
}
return this.session;
}

An update wrapper (which was indeed an update wrapped session) as in the old
implementation is IMO no longer necessary as only the server session holds the
wrapper - if it is a new server session we also must provide a new wrapper.

To avoid or at least recognize manipulation of the session attribute it is maybe
possible to let Cocoon's HttpSession implement HttpSessionAttributeListener and
to check on attributeReplaced(HttpSessionBindingEvent) [1] whether the wrapper
was replaced and not just removed on invalidation of the session. But first this
is no 100%-solution as someone could first remove and later set the attribute
and second it is again overhead (the event is triggered on every session
attribute add/remove/replace).
AFAICS the HTTPSessionBindingListener [2] does not help as there is no type of
HttpSessionBindingEvent, just the information that the wrapper was removed - and
this must happen when the session is invalidated.

WDYT?

Joerg

[1] http://java.sun.com/products/servlet/2.3/javadoc/javax/servlet/http/
HttpSessionAttributeListener.html
#attributeReplaced(javax.servlet.http.HttpSessionBindingEvent)

[2] http://java.sun.com/products/servlet/2.3/javadoc/javax/servlet/http/
HttpSessionBindingListener.html

Re: Re: [IMP] synchronization on session object in Cocoon

2005-05-11 Thread Joerg Heinicke

Nathaniel Alfred Alfred.Nathaniel at swx.com writes:

 AFAIKS is the purpose of the existing synchronized(session) bits to
 protect the consistency of the session object in case it is shared
 between parallel executing requests.

Yes.

 IIUC you need a lock object to really synchronize parallel requests.  
 There I'd say the session is the wrong candidate due to the mentioned
 possible dependency on the servlet 

That's where you and I have different interpretations. The sentence you quoted
is IMO only about information hiding between different web applications. It does
not say anything about if it is guaranteed to have exactly one session object or
not.

Another sentence is the following [1]: The servlet container uses *this
interface* to create *a session* between an HTTP client and an HTTP server. The
*session persists* for a specified time period, *across more than one connection
or page request from the user*.

IMO this means _session_ is not only an abstract real world object, but a Java
object. And you can access the same session object on different requests of the
same (here real world) session.

 I am just afraid that keeping the wrapped sessions in a private map
 may introduce a memory leak.  But maybe WeakHashMap would take care
 of this...

Yes, see Sylvain's proposal or my other mail.

Joerg

[1] http://java.sun.com/products/servlet/2.3/javadoc/javax/servlet/http/
HttpSession.html

RE: Re: [IMP] synchronization on session object in Cocoon

 -Original Message-
 From: news [mailto:[EMAIL PROTECTED] Behalf Of Joerg Heinicke
 Sent: Mittwoch, 11. Mai 2005 17:22
 To: dev@cocoon.apache.org
 Subject: Re: [IMP] synchronization on session object in Cocoon

 I have implemented the session attribute solution. Would be 
 nice if you can
 review it:

 public Session getSession(boolean create) {
 // we must assure a 1:1-mapping of server session to 
 cocoon session
 javax.servlet.http.HttpSession serverSession = 
 this.req.getSession(create);
 if (serverSession != null) {
 synchronized (serverSession) {
 // retrieve existing wrapper
 this.session =
 (HttpSession)serverSession.getAttribute(HTTP_SESSION);
 if (this.session == null) {
 // create wrapper
 this.session = new HttpSession(serverSession);
 serverSession.setAttribute(HTTP_SESSION, 
 this.session);
 }
 }
 } else {
 // invalidate
 this.session = null;
 }
 return this.session;
 }

Looks good to me.

Cheers, Alfred.

This message is for the named person's use only. It may contain confidential, 
proprietary or legally privileged information. No confidentiality or privilege 
is waived or lost by any mistransmission. If you receive this message in error, 
please notify the sender urgently and then immediately delete the message and 
any copies of it from your system. Please also immediately destroy any 
hardcopies of the message. You must not, directly or indirectly, use, disclose, 
distribute, print, or copy any part of this message if you are not the intended 
recipient. The sender's company reserves the right to monitor all e-mail 
communications through their networks. Any views expressed in this message are 
those of the individual sender, except where the message states otherwise and 
the sender is authorised to state them to be the views of the sender's company.

Re: [IMP] synchronization on session object in Cocoon

2005-05-11 Thread Sylvain Wallez

Joerg Heinicke wrote:
Sylvain Wallez sylvain at apache.org writes:
 

I think I have found a more general problem with synchronization in Cocoon. I
tried to solve my problem with synchronization in flow script with an
intermediate object. This object handles the synchronized instantiation of my
component. Unfortunately I found out that synchronized (session) { ... }
still did not work - two requests of the same session run into that block at
the same time. I did some remote debugging. First I thought the reason is in
flow script as it also wraps the session, but it unwraps it later. The reason
is in HttpRequest class. Have a look at the code [1]:
 

 

As you can see on every request a new wrapper is instantiated which is really
bad. It is not possible to synchronize on Cocoon session objects.
 

Wow, very good point!
   

What we probably need is a Map mapping the server sessions to the wrapper
objects.
 

Or more simply we could store the wrapper in the session itself using an 
attribute. That way it would be guaranteed to be created only once.
   

Yes, that's another possibility I also had in mind. But on the one hand this
smells a bit (storing a wrapper in the object that the wrapper wraps), on the
other hand you can not restrict access to it, so it can be manipulated from
somewhere else. But the Map solution can indeed be very resource consuming and a
bottle neck.
I have implemented the session attribute solution. Would be nice if you can
review it:
public Session getSession(boolean create) {
   // we must assure a 1:1-mapping of server session to cocoon session
   javax.servlet.http.HttpSession serverSession = this.req.getSession(create);
   if (serverSession != null) {
   synchronized (serverSession) {
   // retrieve existing wrapper
   this.session =
(HttpSession)serverSession.getAttribute(HTTP_SESSION);
   if (this.session == null) {
   // create wrapper
   this.session = new HttpSession(serverSession);
   serverSession.setAttribute(HTTP_SESSION, this.session);
   }
   }
   } else {
   // invalidate
   this.session = null;
   }
   return this.session;
}
 

This approach has the problem of entering a synchronized block each time 
getSession is called. Although this may not that be much a problem in 
this particular case because it's unlikely that many parallel requests 
exist for a single request, we may totally avoid it except once. We also 
no more need to keep the wrapper as this.session as the servlet 
session stores the wrapper.

public Session getSession(boolean create) {
   javax.servlet.http.HttpSession serverSession = 
this.req.getSession(create);
   if (serverSession != null) {
   Session session = 
(HttpSession)serverSession.getAttribute(HTTP_SESSION);
   if (session = null) {
   return createSessionWrapper(serverSession);
   } else {
   return session;
   }
   } else {
   return null;
   }
}

private Session createSessionWrapper(HttpSession serverSession) {
   synchronized(serverSession) {
   // recheck in the synchronized section
   Session session = 
(HttpSession)serverSession.getAttribute(HTTP_SESSION);
   if (session = null) {
   session = new HttpSession(serverSession);
   serverSession.setAttribute(HTTP_SESSION, session);
   }
   return session;
   }
}

An update wrapper (which was indeed an update wrapped session) as in the old
implementation is IMO no longer necessary as only the server session holds the
wrapper - if it is a new server session we also must provide a new wrapper.
To avoid or at least recognize manipulation of the session attribute it is maybe
possible to let Cocoon's HttpSession implement HttpSessionAttributeListener and
to check on attributeReplaced(HttpSessionBindingEvent) [1] whether the wrapper
was replaced and not just removed on invalidation of the session. But first this
is no 100%-solution as someone could first remove and later set the attribute
and second it is again overhead (the event is triggered on every session
attribute add/remove/replace).
AFAICS the HTTPSessionBindingListener [2] does not help as there is no type of
HttpSessionBindingEvent, just the information that the wrapper was removed - and
this must happen when the session is invalidated.
 

Sorry, what problem are you trying to solve with having the wrapper 
being notified of updates?

Sylvain
--
Sylvain WallezAnyware Technologies
http://apache.org/~sylvainhttp://anyware-tech.com
Apache Software Foundation Member Research  Technology Director

RE: [IMP] synchronization on session object in Cocoon