14 Replies Latest reply on Nov 15, 2004 2:49 AM by belaban

    SERIALIZABLE Bug?  Read of null, put, another thread reading

    tallpsmith

      I'm not sure if this is a bug, or a feature I can't have! :)

      Lets assume a local TreeCache with isolation Level set to Serializable.

      Initiall when the cache is empty, the initial read will of course return null. Lets assume within this same transaction for this first thread that this thread then goes off to get the data from the DB. It holds a Read lock on the item at this point, I assume while it goes to get the data.

      Mean while, another thread comes along and does the same thing. It starts a transaction, reads the value, and finds it null and will do the same thing... Hitting the db.

      However what I would really like is for the second thread to block on the read until the first thread has finished. Basically I want any read or write to hold a write lock only on the ID node in question. Because the node in question is a logical ID of a row in a DB, it is better if the other thread waits for the first, because we only want 1 thread retrieving that value from the db.

      Here's a test case method to show you what I mean:

       public void testRaceConditionOnNotInCacheCondition() throws Exception{
       UserTransaction tx = jotm.getUserTransaction();
      
       tx.begin();
       // we now read the null entry, and decide that we need to go do something.
      
       Object cachedObject = cache_.get("/SecurityInfo/", Integer.toString(23));
       assertNull(cachedObject); // we expect this in this test
      
       /**
       * now start another Thread to go do the same action, looking for the value, but it SHOULD
       * see the result of the main thread put once it commits.
       */
       Thread thread = new Thread(new Runnable() {
      
       public void run() {
       try {
       UserTransaction tx = jotm.getUserTransaction();
       tx.begin();
       log("OtherThread: inspecting the cache");
       Object cachedObject = cache_.get("/SecurityInfo", Integer.toString(23));
      
       log("OtherThread: read from cache:" + cachedObject);
       Thread.sleep(3000);
      
       cachedObject = cache_.get("/SecurityInfo", Integer.toString(23));
      
       log("OtherThread: read(second time) from cache:" + cachedObject);
      
       /**
       * This should really fail because the other thread should actually have put something else there.
       */
       cache_.put("/SecurityInfo", Integer.toString(23), "HelloWorldDIRTY!");
      
       log("OtherThread: Has put something in the cache tha shouldn't be there");
       tx.commit();
       log("OtherThread: Committed... OH my.");
      
       } catch (Exception e) {
       e.printStackTrace();
       }
       log("OthreThread: exiting");
      
       }});
      
       thread.start();
       log("MainThread is now waiting a little bit");
       Thread.sleep(2000); // wait long enough for the other thread to block a bit. Simulate a DB read
       log("MainThread is now putting something in the cache");
       cache_.put("/SecurityInfo", Integer.toString(23), "HelloWorld");
       Thread.sleep(2000); // wait long enough for the other thread to block a bit. Simulate a DB read
       tx.commit();
       log("MainThread: committed");
       thread.join(30000);
      
       log(cache_.get("/SecurityInfo", Integer.toString(23)).toString());
      
      
       }
      


      The output I get when running the above is:

      MainThread is now waiting a little bit
      OtherThread: inspecting the cache
      OtherThread: read from cache:null
      MainThread is now putting something in the cache
      OtherThread: read(second time) from cache:HelloWorld
      OtherThread: Has put something in the cache tha shouldn't be there
      OtherThread: Committed... OH my.
      OthreThread: exiting
      MainThread: committed
      HelloWorldDIRTY!

      This to me should have failed at some level, because the 2nd thread clobbers the first threads put. I want the 2nd thread to block on the first read, and only then read the value the first thread put there.

      Any ideas what's wrong here?

        • 1. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
          belaban

          Thanks to Norbert and Bela for the reply.

          I tested Norberts theory on Thread start position, and it didn't seem to make any difference.

          I took Bela's test case, and this test does work, BUT WITH ONE subtle difference.

          In Bela's test case, he places a value in the cache before starting things.

          This is a _crucial_ difference to mine.

          If I commented out the initial put(..) call before the Readers are created and started, I get the same results as my original test case, Reader 2 actually reads the null while Reader 1 is pausing.

          Here is example output from Bela's original test case:

          main: sleeping for 100
          reader1: about to access tree
          reader1: accessed tree, retval: testvalue
          reader1: sleeping for 3000
          reader2: about to access tree
          reader1: About to commit
          reader1: committed, exiting
          reader2: accessed tree, retval: testvalue
          reader2: About to commit
          reader2: committed, exiting


          The above is exactly what I want, but also occuring when there is null values in the cache. Here's the output when you simply comment out the first put(...) call:

          main: sleeping for 100
          reader1: about to access tree
          reader1: accessed tree, retval: null
          reader1: sleeping for 3000
          reader2: about to access tree
          reader2: accessed tree, retval: null
          reader2: About to commit
          reader2: committed, exiting
          reader1: About to commit
          reader1: committed, exiting


          A null value in the cache seems to be specially treated, as it does not block reads. Unfortunately I need serialized access (read or write) to each leaf node in the tree to guarantee that 2 threads don't try to go off and hit the db and potentially clobber each other.

          Here is the complete test case that I adapted from Bela's original, combining it with the basic shell provided by the org.jboss.test.cache.perf.basic.LocalPerfTestCase class that is provided in the examples src of the distro. You will note that I have added the code to set the cache Isolation level in the initCache() method:


          package com.aconex.jbosscache;
          
          import java.util.Properties;
          
          import javax.naming.Context;
          import javax.naming.InitialContext;
          import javax.transaction.UserTransaction;
          
          import org.jboss.cache.PropertyConfigurator;
          import org.jboss.cache.TreeCache;
          import org.jboss.cache.lock.IsolationLevel;
          import org.jboss.cache.transaction.DummyTransactionManager;
          
          import junit.framework.TestCase;
          
          /**
           * @author psmith
           *
           */
          public class SerializableTests extends TestCase {
           TreeCache cache_;
          
           int cachingMode_ = TreeCache.LOCAL;
          
           final static Properties p_;
          
           // final static Logger log_ = Logger.getLogger(LocalPerfAopTest.class);
           String oldFactory_ = null;
          
           final String FACTORY = "org.jboss.cache.transaction.DummyContextFactory";
          
           DummyTransactionManager tm_;
          
           static {
           p_ = new Properties();
           p_.put(Context.INITIAL_CONTEXT_FACTORY,
           "org.jboss.cache.transaction.DummyContextFactory");
           }
          
           /*
           * @see TestCase#setUp()
           */
           protected void setUp() throws Exception {
           super.setUp();
          
           oldFactory_ = System.getProperty(Context.INITIAL_CONTEXT_FACTORY);
           System.setProperty(Context.INITIAL_CONTEXT_FACTORY, FACTORY);
          
           DummyTransactionManager.getInstance();
           initCaches(TreeCache.LOCAL);
           tm_ = new DummyTransactionManager();
          
           log("LocalPerfAopTest: cacheMode=LOCAL, one cache");
           }
          
           /**
           * @param string
           */
           private void log(String msg) {
           System.out.println(Thread.currentThread().getName() + ": " + msg);
          
           }
          
           /*
           * @see TestCase#tearDown()
           */
           protected void tearDown() throws Exception {
           super.tearDown();
          
           DummyTransactionManager.destroy();
           destroyCaches();
          
           if (oldFactory_ != null) {
           System.setProperty(Context.INITIAL_CONTEXT_FACTORY, oldFactory_);
           oldFactory_ = null;
           }
           }
          
           void initCaches(int caching_mode) throws Exception {
           cachingMode_ = caching_mode;
           cache_ = new TreeCache();
           PropertyConfigurator config = new PropertyConfigurator();
           config.configure(cache_, "META-INF/local-service.xml"); // read in
           // generic local
           // xml
           cache_
           .setTransactionManagerLookupClass("org.jboss.cache.DummyTransactionManagerLookup");
           cache_.setIsolationLevel(IsolationLevel.SERIALIZABLE);
           cache_.startService();
           }
          
           void destroyCaches() throws Exception {
           cache_.stopService();
           cache_ = null;
           }
          
           public void testConcurrentReadsWithSerializableIsolationLevelValueAlreadyInCache()
           throws Exception {
          
           cache_.put("/testfqn", "testkey", "testvalue"); // add initial value, to
           runSerializableTest();
           }
          
           public void testConcurrentReadsWithSerializableIsolationLevelNOValueInCache()
           throws Exception {
          
           runSerializableTest();
           }
          
           /**
           * @throws Exception
           * @throws InterruptedException
           */
           private void runSerializableTest() throws Exception, InterruptedException {
           final long TIMEOUT = 5000;
           Reader r1, r2;
           r1 = new Reader("reader1", 3000);
           r2 = new Reader("reader2", 0);
           r1.start();
           pause(100); // make sure thread1 starts and acquires the lock before
           // thread2
           r2.start();
           r1.join(TIMEOUT);
           r2.join(TIMEOUT);
           }
          
           /**
           * @param i
           * @throws Exception
           */
           private void pause(long i) throws Exception {
           log("sleeping for " + i);
           Thread.sleep(i);
           }
          
           class Reader extends Thread {
           long timeout;
          
           public Reader(String name, long timeout) {
           super(name);
           this.timeout = timeout;
           }
          
           public void run() {
           UserTransaction trans = null;
           try {
           trans = (UserTransaction) new InitialContext(p_)
           .lookup("UserTransaction");
           trans.begin();
           Object retval = null;
           log("about to access tree");
           retval = cache_.get("testfqn", "testkey");
           log("accessed tree, retval: " + retval);
           if (timeout > 0) {
           pause(timeout);
           }
           } catch (Exception e) {
           e.printStackTrace();
           } finally {
           log("About to commit");
           try {
           trans.commit();
           } catch (Throwable t) {
           }
           log("committed, exiting");
           }
           }
           }
          
          }


          I would appreciate any thoughts on this.

          cheers,

          Paul Smith

          • 2. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
            norbert

            actually isolationlevel SERIALIZABLE should result in what you expect to happen since it makes use of EDU.oswego.cs.dl.util.concurrent.FIFOSemaphore wrapped into an Identitylock as implementation of Sync. Thus read-locks are treated the same as write-locks and will block until the end of Transaction as long the previous lock is not owned by the same Globaltransaction.

            Therefore the way your code runs depends on the behaviour of the transactionmanager being used since you start the second Transaction from the thread that is started within the boundaries of the first Transaction. To be independent of TransactionManagerImplementation-specific behaviour try starting the 2nd thread first before opening the first transaction.

            • 3. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
              tallpsmith

              Thanks to Norbert and Bela for the reply.

              I tested Norberts theory on Thread start position, and it didn't seem to make any difference.

              I took Bela's test case, and this test does work, BUT WITH ONE subtle difference.

              In Bela's test case, he places a value in the cache before starting things.

              This is a _crucial_ difference to mine.

              If I commented out the initial put(..) call before the Readers are created and started, I get the same results as my original test case, Reader 2 actually reads the null while Reader 1 is pausing.

              Here is example output from Bela's original test case:

              main: sleeping for 100
              reader1: about to access tree
              reader1: accessed tree, retval: testvalue
              reader1: sleeping for 3000
              reader2: about to access tree
              reader1: About to commit
              reader1: committed, exiting
              reader2: accessed tree, retval: testvalue
              reader2: About to commit
              reader2: committed, exiting


              The above is exactly what I want, but also occuring when there is null values in the cache. Here's the output when you simply comment out the first put(...) call:

              main: sleeping for 100
              reader1: about to access tree
              reader1: accessed tree, retval: null
              reader1: sleeping for 3000
              reader2: about to access tree
              reader2: accessed tree, retval: null
              reader2: About to commit
              reader2: committed, exiting
              reader1: About to commit
              reader1: committed, exiting


              A null value in the cache seems to be specially treated, as it does not block reads. Unfortunately I need serialized access (read or write) to each leaf node in the tree to guarantee that 2 threads don't try to go off and hit the db and potentially clobber each other.

              Here is the complete test case that I adapted from Bela's original, combining it with the basic shell provided by the org.jboss.test.cache.perf.basic.LocalPerfTestCase class that is provided in the examples src of the distro. You will note that I have added the code to set the cache Isolation level in the initCache() method:


              package com.aconex.jbosscache;
              
              import java.util.Properties;
              
              import javax.naming.Context;
              import javax.naming.InitialContext;
              import javax.transaction.UserTransaction;
              
              import org.jboss.cache.PropertyConfigurator;
              import org.jboss.cache.TreeCache;
              import org.jboss.cache.lock.IsolationLevel;
              import org.jboss.cache.transaction.DummyTransactionManager;
              
              import junit.framework.TestCase;
              
              /**
               * @author psmith
               *
               */
              public class SerializableTests extends TestCase {
               TreeCache cache_;
              
               int cachingMode_ = TreeCache.LOCAL;
              
               final static Properties p_;
              
               // final static Logger log_ = Logger.getLogger(LocalPerfAopTest.class);
               String oldFactory_ = null;
              
               final String FACTORY = "org.jboss.cache.transaction.DummyContextFactory";
              
               DummyTransactionManager tm_;
              
               static {
               p_ = new Properties();
               p_.put(Context.INITIAL_CONTEXT_FACTORY,
               "org.jboss.cache.transaction.DummyContextFactory");
               }
              
               /*
               * @see TestCase#setUp()
               */
               protected void setUp() throws Exception {
               super.setUp();
              
               oldFactory_ = System.getProperty(Context.INITIAL_CONTEXT_FACTORY);
               System.setProperty(Context.INITIAL_CONTEXT_FACTORY, FACTORY);
              
               DummyTransactionManager.getInstance();
               initCaches(TreeCache.LOCAL);
               tm_ = new DummyTransactionManager();
              
               log("LocalPerfAopTest: cacheMode=LOCAL, one cache");
               }
              
               /**
               * @param string
               */
               private void log(String msg) {
               System.out.println(Thread.currentThread().getName() + ": " + msg);
              
               }
              
               /*
               * @see TestCase#tearDown()
               */
               protected void tearDown() throws Exception {
               super.tearDown();
              
               DummyTransactionManager.destroy();
               destroyCaches();
              
               if (oldFactory_ != null) {
               System.setProperty(Context.INITIAL_CONTEXT_FACTORY, oldFactory_);
               oldFactory_ = null;
               }
               }
              
               void initCaches(int caching_mode) throws Exception {
               cachingMode_ = caching_mode;
               cache_ = new TreeCache();
               PropertyConfigurator config = new PropertyConfigurator();
               config.configure(cache_, "META-INF/local-service.xml"); // read in
               // generic local
               // xml
               cache_
               .setTransactionManagerLookupClass("org.jboss.cache.DummyTransactionManagerLookup");
               cache_.setIsolationLevel(IsolationLevel.SERIALIZABLE);
               cache_.startService();
               }
              
               void destroyCaches() throws Exception {
               cache_.stopService();
               cache_ = null;
               }
              
               public void testConcurrentReadsWithSerializableIsolationLevelValueAlreadyInCache()
               throws Exception {
              
               cache_.put("/testfqn", "testkey", "testvalue"); // add initial value, to
               runSerializableTest();
               }
              
               public void testConcurrentReadsWithSerializableIsolationLevelNOValueInCache()
               throws Exception {
              
               runSerializableTest();
               }
              
               /**
               * @throws Exception
               * @throws InterruptedException
               */
               private void runSerializableTest() throws Exception, InterruptedException {
               final long TIMEOUT = 5000;
               Reader r1, r2;
               r1 = new Reader("reader1", 3000);
               r2 = new Reader("reader2", 0);
               r1.start();
               pause(100); // make sure thread1 starts and acquires the lock before
               // thread2
               r2.start();
               r1.join(TIMEOUT);
               r2.join(TIMEOUT);
               }
              
               /**
               * @param i
               * @throws Exception
               */
               private void pause(long i) throws Exception {
               log("sleeping for " + i);
               Thread.sleep(i);
               }
              
               class Reader extends Thread {
               long timeout;
              
               public Reader(String name, long timeout) {
               super(name);
               this.timeout = timeout;
               }
              
               public void run() {
               UserTransaction trans = null;
               try {
               trans = (UserTransaction) new InitialContext(p_)
               .lookup("UserTransaction");
               trans.begin();
               Object retval = null;
               log("about to access tree");
               retval = cache_.get("testfqn", "testkey");
               log("accessed tree, retval: " + retval);
               if (timeout > 0) {
               pause(timeout);
               }
               } catch (Exception e) {
               e.printStackTrace();
               } finally {
               log("About to commit");
               try {
               trans.commit();
               } catch (Throwable t) {
               }
               log("committed, exiting");
               }
               }
               }
              
              }


              I would appreciate any thoughts on this.

              cheers,

              Paul Smith

              • 4. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                norbert

                You are right, that is how it's currently implemented. The read operation will not create nodes not existing when there's no Cacheloader defined.

                Thus without any previous 'put' there's not even the root-node to lock.

                • 5. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                  belaban

                  Simple: if the node ("/testfqn") is not there, the get() returns null:

                   public Object _get(Fqn fqn, Object key, boolean sendNodeEvent) throws CacheException {
                   if(log.isTraceEnabled())
                   log.trace("_get(" + ", \"" + fqn + "\", " + key + ", \"" +sendNodeEvent +"\")");
                   try {
                   Node n=findNode(fqn, NODE_READ);
                   if(n == null) return null;
                   if(sendNodeEvent)
                   notifyNodeVisisted(fqn);
                   return n.get(key);
                   }
                   finally {
                   // release locks since no tx context
                   if(locking == true && getCurrentTransaction() == null)
                   releaseLocks(null, fqn);
                   }
                   }
                  


                  This is also clearly stated in the javadoc for get().

                  Bela

                  • 6. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                    belaban

                    Meaning, that you do *not* block on a non-existing node.

                    Bela

                    • 7. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                      belaban

                      Note that even *with* a CacheLoader, a read operation wil never create non-existing parent nodes.

                      Bela

                      • 8. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                        tallpsmith

                        Sure, I can understand why it's doing it, but I was under an impression that it's a violation of the contract for SERIALIZABLE. Am I wrong? (statistics show that I am.. :D)

                        If we pretent do be a DB for just a moment, doesn't SERIALIZABLE dictate that no other connection can create a new row in a table while a SERIALIZABLE transaction is in progress? I could be way wrong here, but I thought that's what the contract was, maybe it's only if one has read something from that table.

                        Is there anyway I can implement the sort of behaviour I need? How can I prevent 2 threads from even looking at the state of a Node in the cache while some other thread has a lock on that Node.

                        What about if I synchronized(..) on the actual Node before inspecting? Could I:

                        Object node = cache.get("/SecurityInfo/" +myId);
                        synchronized(node) {
                        -- read value of node
                        --- do DB Lookup as necessary
                        }

                        If another Thread came along, I would hope it would block. Given that there could me many many many 'myId's', I'd need to evict them after a while, but long enough such that the get(..) call returned the exact same object for multiple threads for the sync call to work.

                        Thanks for all replies. Thoughts would be very much appreciated.

                        • 9. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                          tallpsmith

                          http://www.mssqlcity.com/Articles/General/TIL.htm defines (from SQL Server reference) that SERIALIBLE is as follows:



                          Phantom behavior occurs when a transaction attempts to select a row that
                          does not exist and a second transaction inserts the row before the first
                          transaction finishes. If the row is inserted, the row appears as a phantom
                          to the first transaction, inconsistently appearing and disappearing.


                          In my case, the "Select of a row that does not exist" is the reading of null.


                          • 10. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea

                            The problem here is because the node does not exist so the transaction has nothing to lock against from (lock is aossicated with a node).

                            Bela and I have discussed previously whether *get* a non-existing node should create it implicitly. We have decided against it since it can create an un-expected behavior for exsiting users. But maybe we will re-visit it.

                            Is it possible that you can create the node a priori? That way, the locking should do what you want.

                            On a side note, I have found out that different data stores implement the transaction isolation level (slightly) differently (even among the RDBMS vendors). This is because the data structure (e.g., node vs. table vs. row) and the locking mechanism (optimistic vs. pessimistic).

                            -Ben

                            • 11. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                              tallpsmith

                              Thanks. I can imagine changing the behaviour now would hurt many people. I wonder if it's a optional setting that could be implemented?

                              Anyway, I am thinking of placing a traditional synchronized(...) block initially so that no thread could ever read null.

                              • 12. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                                belaban

                                 

                                "tallpsmith" wrote:

                                If we pretent do be a DB for just a moment, doesn't SERIALIZABLE dictate that no other connection can create a new row in a table while a SERIALIZABLE transaction is in progress?


                                DBs don't need to acquire table locks in all situations, e.g. UPDATEs and READs only acquire row locks, whereas INSERTS, DELETEs and queries need to acquire table locks. Well, I should say this depends on the implementation and of course only applies to pessimistic locking.
                                So the answer to your question would be 'no', because another TX can insert new rows. However, if the current TX tries to do (e.g.) a query, then it needs to acquire the table lock held by the other TX, and will block until released (avoiding phantom reads).


                                Is there anyway I can implement the sort of behaviour I need? How can I prevent 2 threads from even looking at the state of a Node in the cache while some other thread has a lock on that Node.


                                To do that you have to create the node first. Because you cannot lock on a node that doesn't exist. If you want to do a putIfExists(), then I suggest you create a top node /a, and create all subnodes under /a. When you have SERIALIZABLE and navigate down /a, all other TXs will be locked out until you commit.


                                What about if I synchronized(..) on the actual Node before inspecting? Could I:

                                Object node = cache.get("/SecurityInfo/" +myId);
                                synchronized(node) {
                                -- read value of node
                                --- do DB Lookup as necessary
                                }

                                If another Thread came along, I would hope it would block. Given that there could me many many many 'myId's', I'd need to evict them after a while, but long enough such that the get(..) call returned the exact same object for multiple threads for the sync call to work.
                                .


                                Very bad. Better is to make sure /SecurityInfo *always* exists (set eviction policy to *not* remove this one node), then access its subnodes with SERIALIZABLE. Only 1 TX will be able to proceed past the /SecurityInfo 'gate' at any time.

                                This is bad for concurrency though...

                                Hmm, have you looked at CacheLoaders, e.g. JDBCCacheLoader ? This does exactly what you want (if I understand your reqs correctly): whenever a node is not in memory, the CacheLoader will load it from an external store, e.g. a DB. Load and store is synchronized.

                                Bela

                                • 13. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                                  tallpsmith

                                  I had a look at CacheLoader's over the weekend, but not quite sure what I would actually need to implement.

                                  To 'build' the object that needs to be cached requires invoking a custom object which handles all the Data retrieval and building, and because of legacy code, there is _no_ concept of transactions or anything.

                                  This object does not need to be persisted if changed (in fact this object should be immutable, but that's another story entirely), and only needs to be removed when other code deems that any cached value needs to be purged.

                                  I just need to work out which method(s) in CacheLoader are responsible for the object retrieval when not in cache, and I assume the TreeCache can just handle the simple purge/remove situation.

                                  From http://docs.jboss.org/jbcache/TreeCache.html#d0e354:


                                  The CacheLoader interface has a set of methods that are called when no transactions are used: get(), put(), remove() and removeData(): they get/set/remove the value immediately. These methods are described as javadoc comments in the above interface.


                                  I assume these are the methods I need to be concerned with? The above URL mentions other methods which I believe I can basically ignore??? (no-op impl)



                                  • 14. Re: SERIALIZABLE Bug?  Read of null, put, another thread rea
                                    belaban

                                     

                                    "tallpsmith" wrote:


                                    I assume these are the methods I need to be concerned with? The above URL mentions other methods which I believe I can basically ignore??? (no-op impl)




                                    Yes, I'd make them no-ops. If you look at the next version of JBossCache (1.2), we will have a CacheLoaderInterceptor and a CacheStoreInterceptor (check out jboss-head/cache/docs/design/Refactoring.txt). In your case, we could also remove the CacheStoreInterceptor, so the *storing* aspect would never be used.

                                    Bela