Re: Clone problem? Difference in behavior between 1.2 and 1.3
this is not really solveable by wicket itself.
We don't know where such a reference is comming from, so this should be
documented i guess
that if you use session data from the session object itself. You should
always use an extra
indiretion to get it
new Model()
{
getObject() {Session.get().getList()}
}
johan
On 7/10/07, Martijn Dashorst [EMAIL PROTECTED] wrote:
Working on the Book I found a difference between 1.2 and 1.3:
In the following setup if you click on the 'link' a couple of times,
you see that the list in the session grows, and that it is reflected
in each page render.
Now, press the back button and continue clicking on the link. You will
see that the page remains static from that moment on. This was not a
problem in 1.2 because the HttpSessionStore is the only way of storing
the pages for back button behavior. However, in 1.3 the 2nd level
cache is the new way of storing, and this is the consequence of that
store: you get different objects between versions.
Is there anything we can do to prevent this from happening?
public class MySession extends WebSession {
public final ListString list = new ArrayListString();
}
public class FooPage extends WebPage {
public FooPage() {
add(new ListView(foo, MySession.get().list){
protected void populateItem(ListItem item) {
item.add(new Label(text, item.getModelObject()));
}
});
add(new Link(link) {
protected void onClick() {
MySession.get().list.add(foo + System.currentTimeMillis
());
}
});
}
}
html
body
ul
li wicket:id=foospan wicket:id=text/span/li
/ul
a href=# wicket:id=linkClick me/a
/body
/html
--
Wicket joins the Apache Software Foundation as Apache Wicket
Apache Wicket 1.3.0-beta2 is released
Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.3.0-beta2/
Re: [vote] remove type parameter from TextField constructor
Martijn Dashorst wrote: Matej had the brilliant idea of auto discovery of the type parameter for FormComponents when they use a PropertyModel or CompoundPropertyModel. In order to remove confusion for noobs, and to point out that there is a Better Way (tm) I propose the following options: For those wondering if it is still possible to set the target type: yes you can, just use the setType() method on the form component. I know it is a tad late in the game, but I think the clean up is something worthwile. [ ] remove the affected FormComponent constructors with the type parameter [ ] deprecate the affected FormComponent constructors with the type parameter [ ] leave them Ugh. We still seem to be changing lots of fundamental stuff, don't we. :-) I vote: [x] deprecate them for beta3 We could possibly remove them for rc1, but that makes migration from 1.2.x harder, doesn't it? Al -- Alastair Maw Wicket-biased blog at http://herebebeasties.com
Re: [vote] remove type parameter from TextField constructor
Ugh. We still seem to be changing lots of fundamental stuff, don't we. :-) I vote: [x] deprecate them for beta3 We could possibly remove them for rc1, but that makes migration from 1.2.x harder, doesn't it? i personally don't care that much about breaking stuff for 1.3. We really need to streamline the api i think for 1.3 so that we don't have to break a lot in the further anymore lets make 1.3 a very solid base..
Re: [vote] remove type parameter from TextField constructor
Johan Compagner wrote:
this constructor can be deleted yes:
public TextField(final String id, final Class type)
{
super(id);
setType(type);
}
but i don't know about this one:
public TextField(final String id, IModel model, Class type)
{
super(id, model);
setType(type);
}
because if you don't use the propertymodel here then that type is pretty
needed
But you can just call setType() yourself, which is the whole point of
this thread, I thought? (Note that we can possibly improve this further
with generic models in the future, as they are effectively like an
IObjectClassAwareModel.)
And bigger problem here, if you override
public IConverter getConverter(Class type)
and you don't set the type then that converter isn't used.
This sucks. It's totally counter-intuitive.
In my opinion, if you override getConverter() then it should always be
used, even if that means we have to call it with a null type parameter.
(Normally when you're overriding it for a particular component you don't
care about the type parameter anyway).
I did a live demo at the last London Wicket event where I temporarily
overlooked this and couldn't make it work, which was silly of me. Just
goes to show it's not obvious, though. ;-)
Regards,
Al
--
Alastair Maw
Wicket-biased blog at http://herebebeasties.com
Re: [vote] remove type parameter from TextField constructor
Al Maw wrote: In my opinion, if you override getConverter() then it should always be used, even if that means we have to call it with a null type parameter. (Normally when you're overriding it for a particular component you don't care about the type parameter anyway). Looking at this further, it seems logical to me that we should remove the convertValue(String[] value) method from FormComponent and push the logic in there into a proper converter, which gets registered for null types. Thoughts? Regards, Al -- Alastair Maw Wicket-biased blog at http://herebebeasties.com
Re: [vote] remove type parameter from TextField constructor
so you always want to call getConverter() and if typename is not set you get a null param? and then all the components that now implemements convertValue(String[] values) should override getConverted(Class) and test for null and if it is null then return a converter which does the same as convertValue does now. we could do that yes. It cleans up the api and makes it more obvious that getConverter() is always called. johan On 7/11/07, Al Maw [EMAIL PROTECTED] wrote: Al Maw wrote: In my opinion, if you override getConverter() then it should always be used, even if that means we have to call it with a null type parameter. (Normally when you're overriding it for a particular component you don't care about the type parameter anyway). Looking at this further, it seems logical to me that we should remove the convertValue(String[] value) method from FormComponent and push the logic in there into a proper converter, which gets registered for null types. Thoughts? Regards, Al -- Alastair Maw Wicket-biased blog at http://herebebeasties.com
wicketstuff-push: centralizing requests
Hi, I'm currently using wicketstuff-push to push events to a page and it works pretty well, but not well enough. Before trying to move to a comet based implementation, I would like to improve the timer based implementation. In the current implementation you got as many AjaxTimer as you install the push on components on the same page. So if you have three components requiring push, you will have three requests each x seconds, to poll for push events from the three components. This would be much more efficient if I had only one request polling for all push events sent by all components on the page... Hence my first thought was to add the timer behavior to the page instead of the component, and verify that the same kind of behavior is not already installed before installing a new one. The problem is that when I install the behavior the component doesn't know its page. So I have to do something at this time, and then later when I know the page make sure that only one behavior is actually rendered. I see two solutions to this problem: - Add one timer behavior to each component (as today), and then when rendering the component use the wasRendered/markRendered methods of IHeaderResponse to render only one of them (using the isEnabled method to disable it if another similar one has already been rendered). But then I will have to update all the non rendered behaviors so that push events sent to them are actually sent to the only behavior which is rendered. - Modify the API of the push service and ask to enable push on a page before actually using push on components on the page. This would actually install a unique timer behavior on the page. Then when installing push on a component I could simply add something which could be located by the unique timer on the page to collect all push events (maybe a disabled behavior) Now that I expose the two options I think I prefer the first one. But I would like to gather feedback from experienced wicket developers to see if I'm not missing something or if you have better ideas about how to implement what I want. Thanks, Xavier -- Xavier Hanin - Independent Java Consultant http://xhab.blogspot.com/ http://incubator.apache.org/ivy/ http://www.xoocode.org/
Re: [vote] remove type parameter from TextField constructor
On Wednesday, July 11, 2007, 9:19:51 AM, Johan [EMAIL PROTECTED] wrote: Ugh. We still seem to be changing lots of fundamental stuff, don't we. :-) I vote: [x] deprecate them for beta3 We could possibly remove them for rc1, but that makes migration from 1.2.x harder, doesn't it? i personally don't care that much about breaking stuff for 1.3. We really need to streamline the api i think for 1.3 so that we don't have to break a lot in the further anymore lets make 1.3 a very solid base.. I do - that's the reason that we /always/ slip releases, as there's /always/ just one more let's tidy this up change. It also means that committers don't feel any big urge to ship releases, as they're still tweaking as they want, while normal users are still waiting for a stable release they can use. /Gwyn
Re: [vote] remove type parameter from TextField constructor
On 7/11/07, Gwyn Evans [EMAIL PROTECTED] wrote: I do - that's the reason that we /always/ slip releases, as there's /always/ just one more let's tidy this up change. It also means that committers don't feel any big urge to ship releases, as they're still tweaking as they want, while normal users are still waiting for a stable release they can use. I agree, but do have a slightly different pov: I discovered this while revising a chapter for the Book, and not having to explain the type parameter in this chapter saved about 1 1/2 page. I can now delay that until later in the book. In that light, I found the parameter to be superfluous and not needed. I don't mind it being shot down though: like I said, it is late in the game. Martijn -- Wicket joins the Apache Software Foundation as Apache Wicket Apache Wicket 1.3.0-beta2 is released Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.3.0-beta2/
Re: wicketstuff-push: centralizing requests
Xavier Hanin wrote: Hi, I'm currently using wicketstuff-push to push events to a page and it works pretty well, but not well enough. Before trying to move to a comet based implementation, I would like to improve the timer based implementation. In the current implementation you got as many AjaxTimer as you install the push on components on the same page. So if you have three components requiring push, you will have three requests each x seconds, to poll for push events from the three components. This would be much more efficient if I had only one request polling for all push events sent by all components on the page... Hence my first thought was to add the timer behavior to the page instead of the component, and verify that the same kind of behavior is not already installed before installing a new one. The problem is that when I install the behavior the component doesn't know its page. So I have to do something at this time, and then later when I know the page make sure that only one behavior is actually rendered. I see two solutions to this problem: - Add one timer behavior to each component (as today), and then when rendering the component use the wasRendered/markRendered methods of IHeaderResponse to render only one of them (using the isEnabled method to disable it if another similar one has already been rendered). But then I will have to update all the non rendered behaviors so that push events sent to them are actually sent to the only behavior which is rendered. - Modify the API of the push service and ask to enable push on a page before actually using push on components on the page. This would actually install a unique timer behavior on the page. Then when installing push on a component I could simply add something which could be located by the unique timer on the page to collect all push events (maybe a disabled behavior) Now that I expose the two options I think I prefer the first one. But I would like to gather feedback from experienced wicket developers to see if I'm not missing something or if you have better ideas about how to implement what I want. Hy Xavier, I've got an idea but I don't know if it will be convenient: TimerPushBehavior should not extends AjaxTimerBehavior. TimerPushBehavior could now do : - Adding a javascript(jsMain) in the header (using a static id - render once). This javascript can manage ajax timer on a specific callback url. - Each TimerPushBehavior registers its component id on the previous added javascript(jsMain) - When a callback is received by the first added behavior: * get all id registered in jsMain (sent to the behavior as parameters of the callbackUrl) * look for each child(server side) by they component id (IVisitor) * look for TimerPushBehavior on each child * call onTimer method on each TimerPushBehavior of each Child it is may be a little bit hacked but it can be a solution. WDYT? -- Vincent Thanks, Xavier
Re: wicketstuff-push: centralizing requests
On 7/11/07, Vincent Demay [EMAIL PROTECTED] wrote: Xavier Hanin wrote: Hi, I'm currently using wicketstuff-push to push events to a page and it works pretty well, but not well enough. Before trying to move to a comet based implementation, I would like to improve the timer based implementation. In the current implementation you got as many AjaxTimer as you install the push on components on the same page. So if you have three components requiring push, you will have three requests each x seconds, to poll for push events from the three components. This would be much more efficient if I had only one request polling for all push events sent by all components on the page... Hence my first thought was to add the timer behavior to the page instead of the component, and verify that the same kind of behavior is not already installed before installing a new one. The problem is that when I install the behavior the component doesn't know its page. So I have to do something at this time, and then later when I know the page make sure that only one behavior is actually rendered. I see two solutions to this problem: - Add one timer behavior to each component (as today), and then when rendering the component use the wasRendered/markRendered methods of IHeaderResponse to render only one of them (using the isEnabled method to disable it if another similar one has already been rendered). But then I will have to update all the non rendered behaviors so that push events sent to them are actually sent to the only behavior which is rendered. - Modify the API of the push service and ask to enable push on a page before actually using push on components on the page. This would actually install a unique timer behavior on the page. Then when installing push on a component I could simply add something which could be located by the unique timer on the page to collect all push events (maybe a disabled behavior) Now that I expose the two options I think I prefer the first one. But I would like to gather feedback from experienced wicket developers to see if I'm not missing something or if you have better ideas about how to implement what I want. Hy Xavier, I've got an idea but I don't know if it will be convenient: TimerPushBehavior should not extends AjaxTimerBehavior. TimerPushBehavior could now do : - Adding a javascript(jsMain) in the header (using a static id - render once). This javascript can manage ajax timer on a specific callback url. - Each TimerPushBehavior registers its component id on the previous added javascript(jsMain) - When a callback is received by the first added behavior: * get all id registered in jsMain (sent to the behavior as parameters of the callbackUrl) * look for each child(server side) by they component id (IVisitor) * look for TimerPushBehavior on each child * call onTimer method on each TimerPushBehavior of each Child it is may be a little bit hacked but it can be a solution. WDYT? Salut Vincent, The idea to use a specific javascript to handle the timer is interesting. Though ATM I've started implementing the first solution I suggested above, and it now works pretty well like this. So I think I'll stick with that for the moment, unless someone tell me there is a serious flaw in the approach I used. You will also be able to review what I've done when I'll commit it on wicketstuff-push, and revert or use another approach if there is a problem. BTW I'm having an issue with the timer, but I think it's a bug in the AbstractAjaxTimerBehavior implementation (I'll send another e-mail about this). Xavier -- Vincent Thanks, Xavier -- Xavier Hanin - Independent Java Consultant http://xhab.blogspot.com/ http://incubator.apache.org/ivy/ http://www.xoocode.org/
bug in AbstractAjaxTimerBehavior?
Hi, I think I've found a bug in AbstractAjaxTimerBehavior... The problem happens when a component to which an AbstractAjaxTimerBehavior is attached is rendered by an AjaxRequestTarget which is not due to the timer, but to another Ajax call. In this case it seems that the renderHead method called on AbstractAjaxTimerBehavior adds another setTimeout() javascript call than the one originally rendered, which makes the timer call happen twice as frequently as it should. I haven't isolated the problem in a simple example, so my analysis may be wrong (I'm experiencing this issue when working on wicketstuff-push, so the source can be something else). Do you think my analysis make sense? Should I open an issue in JIRA? Xavier -- Xavier Hanin - Independent Java Consultant http://xhab.blogspot.com/ http://incubator.apache.org/ivy/ http://www.xoocode.org/
Re: Escaping quotes in attributes
1. Automatically insert an attribute value that's the same as the name (which I think it always should be), for things like disabled. 2. Prevent people adding null values to the map in the first place. To do this, we'd need to wrap the IValueMap in XmlTag with some magic on the put method to check for null values. Or we could add a boolean flag to IValueMap to say whether it accepts them, or similar. Obviously #1 is nice and simple. What do people think? #1 sounds good to me. Eelco
Re: [vote] remove type parameter from TextField constructor
On 7/11/07, Martijn Dashorst [EMAIL PROTECTED] wrote: On 7/11/07, Gwyn Evans [EMAIL PROTECTED] wrote: I did want to respond to Johan's comment, but it's /not/ a -1 vote, especially in view of your reasons above - 0 if anything. I didn't want to imply as such, but I would not hold a grudge if you would -1 it. It is late in the game, so ... the thing about removing convertValue(String[]) if we are going to do it it needs to be now, or it wont make it into 1.3.xperiod. so we need to decide if this is important enough to get into 1.3 branch. one tricky part about it is that the converter will have to be installed for String[]? or do we call it repeatedly if there is more then one element? -igor Martijn -- Wicket joins the Apache Software Foundation as Apache Wicket Apache Wicket 1.3.0-beta2 is released Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.3.0-beta2/
Wicket and VMBuild
Hi, I'm not on this list, so please reply directly - thanks. Wicket currently have a build set up in vmbuild.apache.org. It's been down for little bit, but is now back up. vmbuild is scheduled to be moved to a faster machine, and I intend to install a more recent build of Continuum (that supports grouping projects and is generally faster, more managable and more stable). I'm able to help get it set up as effectively as possible. There are a lot of failing builds on the machine right now (probably unused by the corresponding projects), so I'm cleaning house before the move. Please let me know if: [ ] you would like the project set up on the new machine with a clean slate [ ] you would like the project (and it's build history if possible) moved over [ ] you are no longer interested in using vmbuild for CI/nightlies/ whatever Thanks! - Brett
Re: bug in AbstractAjaxTimerBehavior?
On 7/11/07, Johan Compagner [EMAIL PROTECTED] wrote: please make a jira issue for this yes, i guess we need some check on an id so that we dont add it twice Done: https://issues.apache.org/jira/browse/WICKET-745 Xavier On 7/11/07, Xavier Hanin [EMAIL PROTECTED] wrote: Hi, I think I've found a bug in AbstractAjaxTimerBehavior... The problem happens when a component to which an AbstractAjaxTimerBehavior is attached is rendered by an AjaxRequestTarget which is not due to the timer, but to another Ajax call. In this case it seems that the renderHead method called on AbstractAjaxTimerBehavior adds another setTimeout() javascript call than the one originally rendered, which makes the timer call happen twice as frequently as it should. I haven't isolated the problem in a simple example, so my analysis may be wrong (I'm experiencing this issue when working on wicketstuff-push, so the source can be something else). Do you think my analysis make sense? Should I open an issue in JIRA? Xavier -- Xavier Hanin - Independent Java Consultant http://xhab.blogspot.com/ http://incubator.apache.org/ivy/ http://www.xoocode.org/ -- Xavier Hanin - Independent Java Consultant http://xhab.blogspot.com/ http://incubator.apache.org/ivy/ http://www.xoocode.org/
Re: Clone problem? Difference in behavior between 1.2 and 1.3
this is not really solveable by wicket itself.
We don't know where such a reference is comming from, so this should be
documented i guess
that if you use session data from the session object itself. You should
always use an extra
indiretion to get it
new Model()
{
getObject() {Session.get().getList()}
}
I don't see a generic solution either, but in this case I though you
should use custom serialization of either your custom session or a
model you use for the extra indirection.
Eelco
Thread synchronization problems in FilePageStore
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();
}
Re: [vote] remove type parameter from TextField constructor
it must handle string[] because sometimes for a multiselect it returns a collection. I guess that is tricky thing to put into the converter interface :( On 7/11/07, Igor Vaynberg [EMAIL PROTECTED] wrote: On 7/11/07, Martijn Dashorst [EMAIL PROTECTED] wrote: On 7/11/07, Gwyn Evans [EMAIL PROTECTED] wrote: I did want to respond to Johan's comment, but it's /not/ a -1 vote, especially in view of your reasons above - 0 if anything. I didn't want to imply as such, but I would not hold a grudge if you would -1 it. It is late in the game, so ... the thing about removing convertValue(String[]) if we are going to do it it needs to be now, or it wont make it into 1.3.xperiod. so we need to decide if this is important enough to get into 1.3 branch. one tricky part about it is that the converter will have to be installed for String[]? or do we call it repeatedly if there is more then one element? -igor Martijn -- Wicket joins the Apache Software Foundation as Apache Wicket Apache Wicket 1.3.0-beta2 is released Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.3.0-beta2/
Re: Clone problem? Difference in behavior between 1.2 and 1.3
the problem with session data is just tricky, if you have a shopping
basket that is stored in the session object, and you add some to it,
the remove a few, then use the back button. what you now see on the
screen is the full shopping basket again, but on the serverside a
previos page is restored, so the view shows everything but the data
not, if you then remove one you have very funny behavior which you
really have to program for.
On 7/11/07, Eelco Hillenius [EMAIL PROTECTED] wrote:
this is not really solveable by wicket itself.
We don't know where such a reference is comming from, so this should be
documented i guess
that if you use session data from the session object itself. You should
always use an extra
indiretion to get it
new Model()
{
getObject() {Session.get().getList()}
}
I don't see a generic solution either, but in this case I though you
should use custom serialization of either your custom session or a
model you use for the extra indirection.
Eelco
Re: Thread synchronization problems in FilePageStore
I opened an issue here http://issues.apache.org/jira/browse/WICKET-746 Eelco
Re: Thread synchronization problems in FilePageStore
Btw, I've tried the test you provided with both FilePageStore and
DiskPageStore. It failed with FilePageStore but passed with
DiskPageStore.
-Matej
On 7/11/07, Jan Vermeulen [EMAIL PROTECTED] wrote:
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);
Re: Nonsensical default validation messages
As long as you're fixing those messages, I think they have a couple
of other problems (I'm using 1.2, but it looks like 1.3 has the same
issues).
First, both messages are off by one . When you create a
MaximumValidator with a value of 3, the value is inclusive so 3 is a
valid input. But the message says '5 must be *smaller than* 3', which
doesn't include 3 itself. It should read 5 must not be greater than
3 or something along those lines.
The second point is minor, but greater and smaller are kind of
lopsided antonyms. Bigger/smaller or greater/less would be more
in sync. Something like this:
NumberValidator.minimum='${label}' must not be less than ${minimum}.
NumberValidator.maximum='${label}' must not be greater than ${maximum}.
Thanks,
-Ryan
On Jul 6, 2007, at 11:42 AM, Al Maw wrote:
The current default validation messages in Application.properties
make no logical sense.
At the moment, if you put in a value that is larger than a
NumberValidator maximum it says:
5 must be smaller than 3
Which it can't be, not even for very small values of 5.
To fix this, we need to do one of the following two things:
1. Change ${input} to ${label}, in which case they would all make
sense.
2. Change the text so it actually makes sense in the context of $
{input}
as in the attached patch. e.g. 5 is larger than 3., which would
at least tell you what's currently wrong with it.
I appreciate the argument that people don't call setLabel() when
they should do, but the current set-up isn't nice.
Thoughts?
Regards,
Al
--
Alastair Maw
Wicket-biased blog at http://herebebeasties.com
Index: /home/almaw/svn/wicket/wicket-1.x/jdk-1.4/wicket/src/main/
java/org/apache/wicket/Application.properties
===
--- /home/almaw/svn/wicket/wicket-1.x/jdk-1.4/wicket/src/main/java/
org/apache/wicket/Application.properties (revision 553981)
+++ /home/almaw/svn/wicket/wicket-1.x/jdk-1.4/wicket/src/main/java/
org/apache/wicket/Application.properties (working copy)
@@ -15,18 +15,18 @@
Required=Field '${label}' is required.
IConverter='${input}' is not a valid ${type}.
-NumberValidator.range=${input} must be between ${minimum} and $
{maximum}.
-NumberValidator.minimum='${input}' must be greater than ${minimum}.
-NumberValidator.maximum='${input}' must be smaller than ${maximum}.
+NumberValidator.range=${input} is not between ${minimum} and $
{maximum}.
+NumberValidator.minimum='${input}' is smaller than ${minimum}.
+NumberValidator.maximum='${input}' is larger than ${maximum}.
-StringValidator.range='${input}' must be between ${minimum} and $
{maximum} characters.
-StringValidator.minimum='${input}' must be at least ${minimum}
characters.
-StringValidator.maximum='${input}' must be at most ${maximum}
characters.
-StringValidator.exact='${input}' must be ${exact} characters long.
+StringValidator.range='${input}' is not between ${minimum} and $
{maximum} characters.
+StringValidator.minimum='${input}' is less than ${minimum}
characters.
+StringValidator.maximum='${input}' is more than ${maximum}
characters.
+StringValidator.exact='${input}' is not exactly ${exact}
characters long.
-DateValidator.range='${input}' must be between ${minimum} and $
{maximum}.
-DateValidator.minimum='${input}' must be greater than ${minimum}.
-DateValidator.maximum='${input}' must be smaller than ${maximum}.
+DateValidator.range='${input}' is not between ${minimum} and $
{maximum}.
+DateValidator.minimum='${input}' is less than ${minimum}.
+DateValidator.maximum='${input}' is larger than ${maximum}.
