My Idiotic Move...

So I had encountered a bug during a data entry session. I had entered about 50 stamps from India, and then as I was completing the entries for India, needed to create a new country for the Indian feudal state of "Gwailor". Well, I added the new country, and closed my application. I just happened to forget, to add some other stamps I had purchased over the weekend and thus reopened my application. To my dismay, all the India stamps I had entered had become "Indian States - Gwailor"? This looked like a bug, but potentially not an obvious one. Why would creating and persisting 50 odd stamps with a reference to a country (India) which clearly had a unique identifier all of sudden become (Indian States - Gwailor). Well it turned out, that during my overzealous striving to minimize and re-use persistence setup code, I had made the EntityManager ThreadLocal. I also discovered I was not actually closing the EntityManager. Thus the entity manager was only being closed on application closure, which caused everything to be flushed from memory to the DB. Somehow (and I think this is TopLink bug, but would be hard to validate) the creation of the new country swapped out the persistent reference to the other country in the stamps. I had put a trace on this and was 100% sure it was not in my service code where this was happening. After much humming and hahhing, I decided on a little refactor. The first was to move the Transaction processing from the ServerFactory into a separate TransactionHandler. Each of my services maintained an instance of the TransactionHandler. The second thing I did is rather than imbedding the obtaining of the EntityManager within the transaction code, I added to each persistent method. Afterall, the creation of an EntityManager is lightweight (as long as you have a cached EntityFactory). I can then pass the EntityManager to the TransactionHandler and it can manage the inner/outer transaction context. While I could have simply have inner transactions, I only want to flush and send events from the top-most call. This means, if a method such as "setAsPrimary( CatalogueNumber catNum, Stamp s )" calls the save( ) for Stamp on the StampService, and this method calls the super.save( ) in PersistenceService, then I only want the save( ) in the StampService to emit the event and flush the EntityManager prior to commit. Either way, I now have the nice TransactionHandler which has a simple "begin( EntityManager em )" method. Only the first call to this method in the stack trace will actually start a transaction (tied to the first EntityManagers). Subsequent calls will merely return the inital calls EntityManager so operations within that transaction are called against same EntityManager. The API for this class is pretty simple: public EntityManager begin( EntityManager em ) throws PersistenceException; public void commit( ); public void rollback( ); public boolean isOuterTransaction( ); protected void validateThreadContext( ); The latter method is used on the begin to validate the the service instance is not being used in a non-thread safe way, and will throw an IllegalStateException if the TransitionHandler thread ID does not match the current thread ID. (While this shouldn't happen, I was concerned that I could have some older areas of code that was calling services in a non-thread safe manner. This ensures that is not happening). So far, this refactoring (while changing a lot of methods) actually went surprisingly clean, and was easy to verify its success with the unit tests I had. I actually was able to add some additional testing including a test that was validating this scenario I described above.