RE: Synchronization on session object (was svn commit: r169856 - /cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/environment/http/HttpRequest.java)

[Nathaniel Alfred <Al...@swx.com>]

> -----Original Message-----
> From: Ralph Goers [mailto:Ralph.Goers@dslextreme.com]
> Sent: Freitag, 13. Mai 2005 08:57
> To: dev@cocoon.apache.org
> Subject: Re: svn commit: r169856 - /cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/environment/http/HttpRequest.java

> >
> >You don't want to replace the outer synchronized(serverSession) by 
> >synchronized(sessions) because that blocks all threads without being
> >necessary (although the effect will be immeasurable).
> >  
> >
> Yes, I do. The alternative is to synchronize on the servlet container's 
> session object, which could have far more horrifying results. Do you 
> have any idea how that will impact the container?  I don't because I 
> cannot know.  For all I know it is conceivable that doing that could 
> cause a deadlock in some wierd scenario.  I also fail to see how locking 
> the map causes any more of a performance hit than locking the session. 
> Since the map is only used by this one method its effect should be far 
> less of an impact that locking the session.  Besides, you ARE blocking 
> all threads on the get and put anyway, since it has been declared a 
> synchronized hash map. In fact, it is being done twice inside of a 
> synchronized block.

Global synchronization on sessions saves two lock operations and gives 
better single-threaded performance.

Local synchronization on serverSession/sessions.get/sessions.put gives
better multi-threaded concurrency.

Both effects are really minute.  I now tend to favour your proposal to
use the global lock because is saves a lot of brain cycles during code
inspection.

However, I am amazed by your categoric opposition to locking the
serverSession.  The whole story started because Joerg wants to use the 
session object in order to coordinate concurrent requests belonging to 
the same session.  I had a difference of opinion with him as well about
the object identity guarantees in HttpRequest.getSession.

I now read it up in the Servlet specs.  Both 2.3 and 2.4 use the same
wording in SRV.7.7.1 Threading Issues:

  Multiple servlets executing request threads may have active access
  to a single session object at the same time.  The Developer has the
  responsability for synchronizing access to session resources as
  appropriate.

That clearly states that synchronized(serverSession) is allowed and
must be used when necessary.

It does not settle my dispute with Joerg though.  One may read the
first sentence as

  "Concurrent requests for the same session may happen and they
   must all be given the same session object." (Joerg)

or as 

  "Concurrent requests for the same session *may* (but need not)
   be given the same session object." (Alfred)

I think we agree that during the lifetime of a session it is not
necessarily represented always by the same Java object.  A clever
container may move it to another cluster node or backing store,
and can hardly be expected to restore it into the same object.

If there is no guarantee that the session object stays the same
between sequential requests, why should there be such a guarantee
for concurrent requests?  Even if the people doing the specs intended
to provide that guarantee, there are still the implementators to
read it the same way as I do or to mess it up.

For example, in Tomcat's PersistenceManager I can't see any protection 
against two requests racing in swapIn and restoring the same session
into two different Java objects.

So it is a shakey assumption that the session object returned from
the container can be used to coordinate concurrent threads.  It works
in normal environments but there is a small chance that it can fail
for complex environments.  I wouldn't bet my head on it.

Now you may argue that Joerg is not using the container session but
the Cocoon wrapper for which the hashmap guarantees that it is always
the same Java object for the same session.

Well, no, not really.  It depends on how equals() is implemented by 
the container session object.  If it does string compares of the session
ids it is fine.  

If it inherits Object.equals, then you loose because the current version 
will produce a new wrapper for every session object.  Since normally one
does not need to compare session objects for equality I doubt that 
container implementers usually bother to override equals and hashCode.

To be safe one should use the sessions.put(serverSession.getId(), session)
but then I don't know anymore how to use weak references for solving the
memory leak problem.  And does the container react if the request uses
a different session object than intended even if it represents the same
session.

Bottomline:

I think synchronized(session) should never be used as vehicle to 
coordinate concurrent requests because there is no convincing guarantee 
that it is always working as expected.  

Joerg, if you want to do it in your usercode, I don't mind, but please
don't use it in common Cocoon code.  My propesed alternative of
synchronized(session.getId().intern()) may look obscure but at least
it is guaranteed to work.

In my interpretation the old getSession version was already compliant to
Servlet specs.  Whether to keep the new version I am +0.  It avoids a
programming error to manifest in simple environments but in complex
setups it just may shift the error rate from 1/thousand to 1/million -
and this is really one of the worst situations.

But maybe I am just paranoid?

Anybody who knows an authoritative statement on the isolation level
for session attributes?

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: Synchronization on session object (was svn commit: r169856 - /cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/environment/http/HttpRequest.java)

[Joerg Heinicke <jo...@gmx.de>]

On 13.05.2005 11:37, Nathaniel Alfred wrote:

> I think synchronized(session) should never be used as vehicle to 
> coordinate concurrent requests because there is no convincing guarantee 
> that it is always working as expected.  
> 
> Joerg, if you want to do it in your usercode, I don't mind, but please
> don't use it in common Cocoon code.  My propesed alternative of
> synchronized(session.getId().intern()) may look obscure but at least
> it is guaranteed to work.

I think we don't get a final answer to whether synchronized(session) is 
supposed to work or not. Your main concern seems to be complex 
environments like clusters. But how is session.getId().intern() supposed 
to work? Have the cluster nodes running by different JVMs and it does 
neither work. Or am I wrong?

Joerg

Re: Synchronization on session object (was svn commit: r169856 - /cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/environment/http/HttpRequest.java)

[Ralph Goers <Ra...@dslextreme.com>]

Nathaniel Alfred wrote:
    <snip>

> Bottomline:
>
>I think synchronized(session) should never be used as vehicle to 
>coordinate concurrent requests because there is no convincing guarantee 
>that it is always working as expected.  
>  
>
Thanks for doing more research.  Although you put it much more clearly 
than me, this is exactly the statement I was trying to make, but not as 
well.

>Joerg, if you want to do it in your usercode, I don't mind, but please
>don't use it in common Cocoon code.  My propesed alternative of
>synchronized(session.getId().intern()) may look obscure but at least
>it is guaranteed to work.
>
>In my interpretation the old getSession version was already compliant to
>Servlet specs.  Whether to keep the new version I am +0.  It avoids a
>programming error to manifest in simple environments but in complex
>setups it just may shift the error rate from 1/thousand to 1/million -
>and this is really one of the worst situations.
>
>But maybe I am just paranoid?
>  
>
That is not necessarily a bad thing.

>Anybody who knows an authoritative statement on the isolation level
>for session attributes?
>
>Cheers, Alfred.
> 
>
>  
>

Re: Synchronization on session object (was svn commit: r169856 - /cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/environment/http/HttpRequest.java)

[Ralph Goers <Ra...@dslextreme.com>]

Nathaniel Alfred wrote:

>
>Global synchronization on sessions saves two lock operations and gives 
>better single-threaded performance.
>
>Local synchronization on serverSession/sessions.get/sessions.put gives
>better multi-threaded concurrency.
>
>Both effects are really minute.  I now tend to favour your proposal to
>use the global lock because is saves a lot of brain cycles during code
>inspection.
>
>However, I am amazed by your categoric opposition to locking the
>serverSession.  The whole story started because Joerg wants to use the 
>session object in order to coordinate concurrent requests belonging to 
>the same session.  I had a difference of opinion with him as well about
>the object identity guarantees in HttpRequest.getSession.
>
>I now read it up in the Servlet specs.  Both 2.3 and 2.4 use the same
>wording in SRV.7.7.1 Threading Issues:
>
>  Multiple servlets executing request threads may have active access
>  to a single session object at the same time.  The Developer has the
>  responsability for synchronizing access to session resources as
>  appropriate.
>
>That clearly states that synchronized(serverSession) is allowed and
>must be used when necessary.
>
>  
>
I had to comment on this.  "Session reources" are clearly not the same 
thing as the session itself.  The implication here is that a servlet 
must be thread safe and that multiple requests can occur for the same 
session simultaneously. Any objects that are scoped to the session must 
be written to support that.  It doesn't necessarily imply that 
synchronizing on the session is OK (which you pretty much came to the 
conclusion later in the email).

Ralph

Ralph