Thanks, I'll change the "enabled" access to use isEnabled() instead. The idea behind synchronizing the nonfinal, nonvolatile variable set in the Session constructor is that even with the variable read synchronized, the variable value may not be visible to other threads if the set is not synchronized. I could also just call logon() which is synchronized and does effectively the same thing as setting enabled = true. Can you explain more why you thought the synchronization was redundant?

Sorry you're absolutely correct, it does need the sync in the constructor. I was wondering how another thread could get a reference to an instance variable of an object yet to be constructed, but I forgot about it needing to flush back to main memory.

... and on closer inspection, I see that Session registers itself in the static session map in the constructor, so that is how another thread could get a reference to it!

I was doing some reading to try and improve my understanding of issues like this and came across http://www.ibm.com/developerworks/java/library/j-jtp0618.html. It recommends not publishing 'this' references during a constructor. It would be possible to make adding to the session map the SessionFactory's responsibility, but that might break any user SessionFactory implementations. Maybe 2.0?

Yeah, the registration-in-constructor behavior is from the C++ port. I know it's not considered good practice. I like your suggestion about doing it in the factory, but I have the same concerns about altered behavior.

These are tricky issues. Like you mentioned, even without leaking "this" in the constructor, there is an issue of field visibility on multiprocessor machines. I like using test-driven techniques for software development, but many concurrency issues still seem to require visual inspection and analysis. I definitely appreciate another set of eyes to watch for these types of problems.

In this case it was automated code analysis that picked up the problem. I had it look for threading issues since I was having some problems with some of the acceptance tests, but I see you've recently fixed those (locking around sequence number updates). It flagged a few other things, but this was the only one I thought could cause problems.