As a consequence of 'random' errors in a single-page application using
Wicket, that always occurred after restoring a page from the FilePageStore,
I have been debugging the FilePageStore extensively.
I have extended the FilePageStoreTest with a method to simulate the errors
(see code included at the end of this post).
When does the error occur ?
I suppose the error is unlikely to occur in an application in which
navigation is between pages. But in a 'single-page' application, or an
application that allows for state changes within the same page, it is likely
to occur. The error occurs when someone generates a new version of a page,
requests a previous version of that same page, and repeats that 'forward and
backward' movement. Since the page id remains the same, and the
versionNumber gets incremented and decremented, there is a case in which he
ends up with an incorrect restored page, i.e. a page that corresponds to a
previous 'forward and backward' movement: it has the same page id and
versionNumber, but not necessarily the same state.
What's really going wrong here ?
Finally I found 2 'holes' in the thread synchronization between the
different threads that operate in the process of page storage. (1) is more
likely to happen than (2).
(1) The thread that saves the pages in a file, sleeps during 1 second
(before 2, in which the error occurred more easily). Then it gets an
iterator on the 'pagesToBeSaved' map. For being a ConcurrentHashMap, that
iterator takes a snapshot of the contents of the map, and works on that
snapshot. For performance reasons, this process is does not lock the
'pagesToBeSaved' map. So someone can come in an store a new entry in the map
while the iterator is busy saving pages. If the entry happens to have the
same key as one that is already present, it simply replaces that entry.
Problem is now that the saving thread saves the cached (now obsolete) entry,
and finally removes the (updated) entry. So the last added entry never gets
saved, and on the next retrieval, the wrong page will be returned, because
that's what's in the file.
(2) The thread that serializes the pages correctly locks access to the list
of pages to be serialized for a given session, but when it has serialized a
page, it first removes that entry from the 'pagesToBeSerialized' list, and
only after that (without locking) adds that entry to the 'pagesToBeSaved'.
That leaves a small hole where the given entry is in neither of the 2 maps.
With some bad luck (it actually occurred in realtime), the thread that looks
for a page to restore it, might not find it in either of the two lists, and
thus decide to restore it from file. And since there was already some file
with that key, it ends up restoring the wrong page. This is a less likely
scenario to occur, but it can be easily provoked by adding a sleep() between
removal from one list and addition to the other to exagerate the hole.
Solutions to the problem
With the following changes in FilePageStore, the holes can be tapped:
(1) change PageSavingThread to iterate the keys, and lookup actual the value
for each iteration. That way, we reduce the change of saving an obsolete
page if the 'pagesToBeSaved' map has been updated after the iterator has
been created. Advantage: no locking for the whole iteration process is
needed.
Iterator it = pagesToBeSaved.keySet().iterator();
while (it.hasNext())
{
synchronized (pagesToBeSaved) {
SessionPageKey key =
(SessionPageKey)pagesToBeSaved.get(it.next());
if (key.data instanceof byte[])
{
savePage(key, (byte[])key.data);
}
pagesToBeSaved.remove(key);
}
}
(2) add a synchronize lock on the 'pagesToBeSaved' map for each single
iteration, to prevent that another thread can update an entry while it is
being saved and removed. The updates on 'pagesToBeSaved' must also lock
access to the map. This prevents the update of an entry in the map while the
saving thread is saving and removing the entry.
synchronized (pagesToBeSaved) {
pagesToBeSaved.put(key, key);
}
(3) change the order in which the PageSerializingThread removes entries from
the 'pagesToBeSerialized' map and adds entries to the 'pagesToBeSaved' map:
if the entry is added to the 'pagesToBeSaved' before being removed from the
'pagesToBeSerialized', there can be no case in which it is neither of the
two lists. The fact that it might appear in both lists at a single moment,
does not affect the correct restore of pages in any way.
synchronized (sessionList)
{
key.setObject(pageBytes);
synchronized (pagesToBeSaved) {
pagesToBeSaved.put(key, key);
}
sessionList.remove(key);
sessionList.notifyAll();
}
Applying these changes, I have been able to eliminate the errors that
occurred. I tried them out with various 'sleep' times in the unitTest.
Below is the code for the unitTest:
A simple TestPage to be used within the unitTest
package org.apache.wicket.protocol.http;
import org.apache.wicket.markup.html.WebPage;
import org.apache.wicket.model.Model;
class TestPage extends WebPage {
private static final long serialVersionUID = 1L;
public TestPage() {
setModel(new Model());
}
public void setValue(int value) {
// force a version change
setModelObject(new Integer(value));
// force end of version
onDetach();
}
public int getValue() {
return(((Integer)getModelObject()).intValue());
}
public boolean isBookmarkable() {
return(false);
}
public boolean isVersioned() {
return(true);
}
}
the extended FilePageStoreTest
package org.apache.wicket.protocol.http;
import org.apache.wicket.Application;
import org.apache.wicket.IPageMap;
import org.apache.wicket.Session;
import org.apache.wicket.WicketTestCase;
import
org.apache.wicket.protocol.http.SecondLevelCacheSessionStore.IPageStore;
/**
* @author jcompagner
*/
public class FilePageStoreTest extends WicketTestCase
{
...
private void sleep(int time) {
try
{
Thread.sleep(time);
}
catch (InterruptedException e)
{
throw new RuntimeException(e);
}
}
/**
* This method simulates a situation in which 2 versions of a page are
created,
* and 2 subsequent 'backs' restore the previous versions of that page,
* ending in the restore of the original page.
*
* When repeating this process a number of times, and if the response
times
are
* below the cycle times of the serialization and save threads of the
FilePageStore,
* synchronization errors occur: since the version numbers overlap
between
the loops
* (version 1 for loop 2 is stored with the same key as version 1 for
loop
1),
* there is a case where the wrong page is restored: it returns the page
with
* the same id and versionNumber from a previous loop.
*
* To detect that problem, a value unique to the loop is added to the
page,
* and when restored, it is checked if the correct value is in the
restored
page.
*
* Time between requests and number of loops can be variated to simulate
the problem.
*/
public void testRestorePage() {
// time to wait between subsequent 'requests' (miliseconds)
int time = 5;
// number of loops
int loops = 100;
// creation of a non-bookmarkable and versioned page
TestPage page = new TestPage();
tester.setupRequestAndResponse();
Session session = Session.get();
IPageStore pageStore =
((SecondLevelCacheSessionStore)Application.get().getSessionStore()).getStore();
// pagemap is needed for storage
// (without that, the versionManager never increments the
versionNumber
// because getLastVersion() is based on a cache in the pageMap)
IPageMap pageMap = page.getPageMap();
// store the original version 0
page.setValue(0);
pageMap.put(page);
sleep(time);
for (int i = 1; i <= loops; i++) {
// first requestCycle: change value & store version 1
pageStore.pageAccessed(session.getId(),page);
page.setValue(i);
pageMap.put(page);
sleep(time);
// second requestCycle: change value & store version 2
pageStore.pageAccessed(session.getId(),page);
page.setValue(0);
pageMap.put(page);
sleep(time);
// third requestCycle: restore version 1 (expected
value: i)
pageStore.pageAccessed(session.getId(),page);
page =
(TestPage)pageStore.getPage(session.getId(),null,0,1,0);
assertNotNull("loop "+i+": no page found for version
1",page);
assertEquals("loop "+i+": incorrect page restore for
version
1:",i,page.getValue());
pageMap.put(page);
sleep(time);
// fourth requestCycle: restore version 0 (expected
value: 0)
pageStore.pageAccessed(session.getId(),page);
page =
(TestPage)pageStore.getPage(session.getId(),null,0,0,0);
assertNotNull("loop "+i+": no page found for version
0",page);
assertEquals("loop "+i+": incorrect page restore for
version
0:",0,page.getValue());
pageMap.put(page);
sleep(time);
}
}
--
View this message in context:
http://www.nabble.com/Thread-synchronization-problems-in-FilePageStore-tf4063498.html#a11545253
Sent from the Wicket - Dev mailing list archive at Nabble.com.
