public void expire(boolean notify) { // Mark this session as "being expired" if needed if (expiring) return; // No man's land here synchronized (this) { if (manager == null) return; expiring = true;
Have you run into any issues with this, or was it just a random code inspection?
Just to butt in here. The easiest fix is to move or copy the if(expiring) inside the synchronized() section. Does such a large block have to be synchronized(this) {} ? I can't see any other code using synchronized(this) in the class (or synchronized(session) in the package), so I guess the only thing being protected is the if(expiring==false) { expiring=true; } test and set. As things stand now is the StandardSession author sure there is no issue of needing to make the JVM perform a memory write barrier ? To ensure that expiring=true is flushed for other threads see it immediately. Otherwise the JVM maybe free to optimize (and defer) this write to memory (unless you start getting into declaring 'expiring' volatile). Suggestion: if (expiring) return; synchronized (this) { if (expiring) return; if (manager == null) return; expiring = true; } // From here is no need to keep the lock AFAIKS and by closing the synchronized() we will ensure memory write barrier. // Towards the end of the function it sets expiring=false; I dont think the any code in critical to seeing the expiring==false again, not once isValid==false in anycase.
This has been fixed in trunk and proposed for 6.0.x and 5.5.x
This has been fixed in 6.0.x and 5.5.x and will be included in 6.0.21 and 5.5.28 onwards.