On Tue, Jun 21, 2011 at 12:52 PM, Juergen Donnerstag
<[email protected]> wrote:
> mind you providing a test case to investigate why it's failing. I
> don't want to say it is the reason, but may be WiQuery is using it the
> wrong way and their devs just didn't detect it yet.
The newly introduced state/assertions try to prevent
ConcurrentModificationExceptions when adding listeners to the target
while the listeners are being processed. While this is a laudable
goal, it fails when something that just works without causing
ConcurrentModificationExceptions now suddenly is forbidden.
That said, WiQuery does the following to ensure that a component does
the JQuery initialization dance. The javascript js is something like
"$('id').activatePlugin();"
AjaxRequestTarget ajaxRequestTarget = (AjaxRequestTarget) requestHandler;
ajaxRequestTarget.addListener(new IListener() {
public void onAfterRespond(Map<String, Component> map,
IJavaScriptResponse response) {
response.addJavaScript(js);
}
public void onBeforeRespond(Map<String, Component> map,
AjaxRequestTarget target) {
// Do nothing
}
});
This is (ultimately) called from AjaxRequestTarget#renderHead().
Now I am quite positive that this is a bit of over-engineering/left
overs from 1.4 days on WiQuery's part, according to me the following
should also work:
AjaxRequestTarget ajaxRequestTarget = (AjaxRequestTarget) requestHandler;
requestHandler.getHeaderResponse().renderOnDomReadyJavaScript(js);
Given that WiQuery probably does something stupid (the simplified code
suggests such), I do think that the current implementation of
restricting additions to the listeners is too strict.
There are a couple of things we can change:
1. relax the assertions such that they only cover the cases where
things actually go wrong, or
2. use CopyOnWriteArraySet instead of HashSet to make iteration safe
even when things get added to the set, or
3. ...?
Number 2 sounds like a good idea, but might introduce errors where
someone is adding a listener which doesn't get processed since the
iterator works on a previous copy—hard to debug, but quite easy to
track (keep a previous size and compare/assert after the loop). It
also consumes more memory during request processing.
I agree with the issue that a ConcurrentModificationException is less
descriptive than asserting that things are set in stone. So I'd like
to opt for number 1 and relax the assertions to just those spots where
things go wrong, *and* properly document it instead of relying on
runtime exceptions that may/may not occur.
Martijn
